From ab9abb52ef5dfd03f3a34ac31035d0a5298becd2 Mon Sep 17 00:00:00 2001 From: hanafish <1106510024@qq.com> Date: Tue, 4 Aug 2026 13:08:04 +0800 Subject: [PATCH] docs(skills): modularize governance references --- .gitignore | 4 + .orgii/skills/architecture-audit/SKILL.md | 593 +----------------- .../references/acceptance-criteria.md | 58 ++ .../references/audit-layers.md | 200 ++++++ .../references/failure-patterns.md | 137 ++++ .../references/planning-and-execution.md | 87 +++ .../references/systematic-sweeps.md | 86 +++ .../dual-instance-verification/SKILL.md | 212 +------ .../references/failure-taxonomy.md | 75 +++ .../references/invariant-determinism.md | 39 ++ .../references/ledger-commands.md | 19 + .orgii/skills/e2e-testing/SKILL.md | 267 +------- .../skills/e2e-testing/references/commands.md | 43 ++ .../e2e-testing/references/core-ui-policy.md | 41 ++ .../references/lifecycle-and-reload.md | 19 + .../references/orchestration-and-diff.md | 60 ++ .../references/rust-runtime-policy.md | 30 + .../references/workspace-and-matrix.md | 57 ++ .orgii/skills/org2-performance-guard/SKILL.md | 142 +---- .../references/runtime-patterns.md | 45 ++ .../references/surface-and-lifecycle.md | 38 ++ .../references/verification-and-delivery.md | 54 ++ .orgii/skills/react-best-practices/SKILL.md | 223 +------ .../references/compatibility-boundaries.md | 16 + .../references/high-risk-surfaces.md | 17 + .../references/implementation-guidance.md | 87 +++ .../references/workflow-and-verification.md | 35 ++ 27 files changed, 1350 insertions(+), 1334 deletions(-) create mode 100644 .orgii/skills/architecture-audit/references/acceptance-criteria.md create mode 100644 .orgii/skills/architecture-audit/references/audit-layers.md create mode 100644 .orgii/skills/architecture-audit/references/failure-patterns.md create mode 100644 .orgii/skills/architecture-audit/references/planning-and-execution.md create mode 100644 .orgii/skills/architecture-audit/references/systematic-sweeps.md create mode 100644 .orgii/skills/dual-instance-verification/references/failure-taxonomy.md create mode 100644 .orgii/skills/dual-instance-verification/references/invariant-determinism.md create mode 100644 .orgii/skills/dual-instance-verification/references/ledger-commands.md create mode 100644 .orgii/skills/e2e-testing/references/commands.md create mode 100644 .orgii/skills/e2e-testing/references/core-ui-policy.md create mode 100644 .orgii/skills/e2e-testing/references/lifecycle-and-reload.md create mode 100644 .orgii/skills/e2e-testing/references/orchestration-and-diff.md create mode 100644 .orgii/skills/e2e-testing/references/rust-runtime-policy.md create mode 100644 .orgii/skills/e2e-testing/references/workspace-and-matrix.md create mode 100644 .orgii/skills/org2-performance-guard/references/runtime-patterns.md create mode 100644 .orgii/skills/org2-performance-guard/references/surface-and-lifecycle.md create mode 100644 .orgii/skills/org2-performance-guard/references/verification-and-delivery.md create mode 100644 .orgii/skills/react-best-practices/references/compatibility-boundaries.md create mode 100644 .orgii/skills/react-best-practices/references/high-risk-surfaces.md create mode 100644 .orgii/skills/react-best-practices/references/implementation-guidance.md create mode 100644 .orgii/skills/react-best-practices/references/workflow-and-verification.md diff --git a/.gitignore b/.gitignore index 9afbcb516..682a8a3f1 100644 --- a/.gitignore +++ b/.gitignore @@ -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/ diff --git a/.orgii/skills/architecture-audit/SKILL.md b/.orgii/skills/architecture-audit/SKILL.md index ead5ee6b1..15a9a03d1 100644 --- a/.orgii/skills/architecture-audit/SKILL.md +++ b/.orgii/skills/architecture-audit/SKILL.md @@ -1,575 +1,42 @@ --- name: architecture-audit -description: Systematic architecture audit and refactoring methodology for Rust + TypeScript codebases. Use when performing refactoring, cleanup, unification, code review, dead code removal, module reorganization, or tech debt elimination. Ensures no naming confusion, semantic overloading, hidden defaults, duplicate logic, or architectural inconsistencies are missed. +description: Systematic architecture audit and refactoring methodology for Rust and TypeScript codebases. Use before finalizing refactor, cleanup, unification, dead-code removal, module reorganization, domain rewrite, type/control-flow redesign, or tech-debt plans; also use for reviews involving naming overload, hidden defaults, duplicate logic, wire protocols, entry-point initialization parity, or resolver symmetry. --- -# Architecture Audit & Refactor Methodology +# Architecture Audit -Lessons learned from multiple rounds of agent architecture refactoring where critical issues were repeatedly missed despite "thorough" audits. +Audit from acceptance criteria and authoritative ownership, not from compilation success alone. -| Date | Change | -|------|--------| -| 2026-04 | Added Layers 8–10 (Wire Protocol, Init Parity, Resolver Symmetry) and Systematic Sweep discipline after schemars/proxy token-bloat incident | -| 2026-04 | Added anti-patterns 22–34 (learnings-config refactor: config cohesion, background resolver coupling, fallback parity, session-layer decisions) | -| 2026-05 | Added anti-patterns 35–46 (control-flow, Agent Org finality, task-runtime, schema discipline, E2E false-path, team-mode, queue-progress) | -| 2026-06 | Added anti-patterns 47–54 (queue/turn lifecycle unification: FSM single source of truth, generation counters, cancel semantics, duplicate dispatchers) | +## Core rules -## When To Use +1. Define observable acceptance criteria before proposing or editing code. +2. Trace the complete path across frontend, command/API boundary, core logic, persistence, and external wire payloads. +3. Treat semantic overload, wildcard/default branches, and cross-domain leakage as correctness risks. +4. Compare every initialization entry point and every resolver source with explicit matrices. +5. Inspect serialized output whenever data crosses a process or network boundary. +6. When one defect is found, classify it and sweep the whole relevant scope. +7. Preserve one authoritative owner for mutable state; reject snapshots and shadow booleans that can drift. +8. Separate audit-only findings from implementation; do not silently expand a focused PR. +9. Verify each phase before continuing and run proportional global verification at the end. +10. Report covered layers, intentionally skipped layers, evidence, and remaining risks. -- Before ANY refactoring plan is finalized -- When user reports confusion about code that you previously audited -- When cleaning up a domain (e.g., "unified agent architecture") -- When doing dead code removal or module reorganization +## Workflow -## Core Principle: Acceptance Criteria First +1. Read [acceptance-criteria.md](references/acceptance-criteria.md) before producing a refactor plan or declaring an audit complete. +2. Select the relevant audit layers: + - Read [audit-layers.md](references/audit-layers.md) for architecture, types, naming, defaults, domain boundaries, wire payloads, init parity, or resolver work. + - Read only the layers applicable to the change, but state which of all ten were covered or intentionally skipped. +3. Read [systematic-sweeps.md](references/systematic-sweeps.md) after identifying a repeated defect class or when doing cleanup/unification. +4. Read [planning-and-execution.md](references/planning-and-execution.md) when producing a plan or implementing an approved refactor. +5. Read [failure-patterns.md](references/failure-patterns.md) when reviewing a broad rewrite, investigating an escaped defect, or checking whether a proposed design repeats a known failure mode. -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. +## Delivery -``` -- [ ] 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 -``` - ---- - -## 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. - ---- - -## Plan Structure - -### Phase ordering rules - -1. **Delete dead code first** (Phase 1 always) — reduces noise for all subsequent phases -2. **Unify duplicated logic next** — establishes shared foundations -3. **Structural/naming cleanup last** — cosmetic changes on a clean codebase - -### Phase granularity - -Each phase must be: - -- **Independently verifiable**: `cargo check` passes after each phase -- **Scope-bounded**: affects at most ~20 files -- **Both-sides**: if a Rust change affects frontend types, the frontend change is in the SAME phase - -### Plan anti-patterns - -- "Create abstraction" without "Wire it in" — creates dead code. Every "create" must have "integrate" + "delete old" in same phase. -- Phase marked "complete" without verification — each phase ends with `cargo check --all-targets` + zero warnings. -- Auditing one layer (Rust) but not the other (TypeScript) — audit both together for shared concepts. -- "Future" or "deferred" items — if worth noting, worth doing now or explicitly descoping with user. -- "It compiles, ship it" — compilation says nothing about semantic correctness. -- "Not in my task scope" — always expand audit scope to adjacent systems that share terminology. - ---- - -## Execution Discipline - -### Before each phase - -1. Verify starting state: `cargo check` passes, note warning count -2. Read the files you're about to change (never edit blind) - -### After each phase - -1. `cargo check` — zero errors -2. Warning count must be <= previous (ideally decreasing) -3. For frontend: `tsc --noEmit` or equivalent - -### Global verification (after all phases) - -Run every checklist item. If any fails, the refactor is not complete. - ---- - -## Common Refactoring Patterns - -### Unifying duplicate initialization - -When two code paths do overlapping work: - -1. List every step each path performs (side by side) -2. Mark shared steps vs variant-specific steps -3. Create factory function for shared steps, returns "base" result -4. Each variant calls factory, adds variant-specific work -5. Delete duplicated code from each variant - -### Eliminating dead abstractions - -1. Confirm zero callers (grep + compiler warnings) -2. If abstraction SHOULD be used: integrate it properly -3. If not: delete entirely -4. Never leave "aspirational" code - -### Replacing hardcoded strings with typed constants - -1. Define enum/const in ONE canonical location -2. Add `as_str()` for serialization boundaries -3. Replace ALL occurrences (including tests and comments) -4. Verify zero remaining with grep - -### Introducing an FSM to replace scattered boolean/atom state - -When "is the system in state X?" is answered by reading multiple atoms: - -1. List every atom/boolean that contributes to the answer -2. Define the complete set of mutually-exclusive states (phases) as an enum/union type -3. Write transition functions for each edge (e.g. `beginTurn`, `markRunning`, `markTerminal`, `forceIdle`) -4. Add a monotonically increasing `generation` field; bump it synchronously in every `begin*` transition -5. All signal handlers check `signal.generation === current.generation` before acting -6. Delete the old atoms; derive any needed UI booleans from the FSM phase -7. Verify: grep the codebase for the old atom names — zero remaining reads outside the FSM module - ---- - -## Systematic Sweep Discipline (Added 2026-04) - -**When you find one instance of a problem category, you MUST sweep the entire codebase for all instances before moving on.** - -This was the single biggest failure mode in the 2026-04 audit cycle: fixing one `blocking I/O` site but not scanning for all others, fixing one `error swallowing` pattern but only in JSON/serde contexts. - -### The Rule - -For every issue found: - -1. **Classify it** — what is the general pattern? (e.g., "sync I/O in async fn", "unwrap_or_default hiding errors", "hardcoded string instead of const") -2. **Write a grep pattern** that catches ALL instances of this class, not just the one you found -3. **Run the grep across the entire target scope** (e.g., all of `agent_core/`) -4. **Record the full hit list** before fixing any -5. **Fix ALL instances** or explicitly defer with user agreement - -### Common sweep patterns - -```bash -# Blocking I/O in async context -rg "std::fs::" --type rust -l # then check if callers are async - -# Error-swallowing unwrap_or_default -rg "unwrap_or_default\(\)" --type rust - -# HTTP client construction hiding errors -rg "\.build\(\)\.unwrap_or" --type rust - -# Hardcoded finish_reason strings -rg '"stop"|"tool_calls"|"end_turn"' --type rust - -# Schema generators that may add unwanted fields -rg "SchemaSettings|into_root_schema" --type rust - -# Repeated state lookups in one function (consolidation candidate) -rg "get_session\(&session_id\)" --type rust -c # >1 per file = suspect - -# Guaranteed-Some Option wrappers (ok_or followed by Some()) -rg "ok_or.*\?\s*;" --type rust # then check if result is wrapped in Some() - -# Non-atomic multi-step DB writes (split-brain window) -rg "update_status|upsert_session" --type rust # multiple calls in sequence = candidate for merge - -# DEPRECATED fields still being assigned or read — remove or migrate first -rg -i "deprecated" --type rust -C 3 # then check: is the deprecated item still assigned/read? - -# Types alive only in definition + re-export chains (zombie types) -# For each pub struct: count callers outside its own file + mod.rs re-exports + tests -# If all hits are definition/re-export/test → dead - -# Cross-module naming collisions -# Export every pub struct name, sort, find duplicates across modules -rg "^pub struct " --type rust -l # list files, then grep struct names across all -``` - -### TypeScript/JavaScript sweep patterns - -```bash -# TypeScript: atoms serving multiple concerns -rg "Atom\b" --type ts -l # list files, then check each atom name for conjunctions - -# TypeScript: event handlers directly setting runtime status -rg "setRuntimeStatus|setIsRunning|isRunning\s*=" --type ts - -# TypeScript: duplicate send paths (direct transport calls outside dispatcher) -rg "dispatchMessage|sendMessage" --type ts -l # >1 file calling transport = suspect - -# TypeScript: UI components importing transport/dispatch directly -rg "from.*dispatcher|from.*transport" --type ts # should only appear in the dispatcher file - -# TypeScript: atoms reset in multiple places for different concerns -rg "set\(.*Atom.*false\)" --type ts # find atoms cleared in multiple locations -``` - -### Anti-pattern: "Fix the one, forget the class" - -``` -Round 1: Found blocking I/O in memory/commands.rs. Fixed it. Declared "blocking I/O: done." -Round 2: Found blocking I/O in init_helpers.rs, channel.rs, prompt_sections.rs, prompt_helpers.rs. - -Why? Because round 1 only fixed the reported instance, never swept for the pattern. -``` - ---- - -## Anti-Patterns That Caused Missed Issues - -1. **"It compiles, ship it"** — `_ => OsPromptBuilder` compiles perfectly but gives Custom agents the wrong identity. Compilation correctness != semantic correctness. - -2. **"Not in my task scope"** — Provider naming was missed because task was "unify agents". Always expand audit to adjacent systems sharing terminology. - -3. **"Grep-and-skim"** — Searching `AgentVariant::Os` finds explicit uses but misses `_ =>` branches. Read the logic, not just pattern matches. - -4. **"Fix what's reported, not what's wrong"** — Fixing variant branches is shallow. The deeper issue (prompts fundamentally different, init 80% duplicated) requires reading full code paths. - -5. **"One more pass will catch everything"** — Same mental model finds same category of issues. Use different audit lenses (the 7 layers) to find different categories. - -6. **"Fix the one, forget the class"** (Added 2026-04) — Finding one blocking I/O site and fixing only that site. The correct response is: classify the pattern, grep the entire codebase, fix ALL instances. See "Systematic Sweep Discipline" above. - -7. **"Source looks fine, must be fine"** (Added 2026-04) — `schemars::openapi3()` looks like a perfectly reasonable API call. The bug is in the OUTPUT, not the source. For anything that crosses a network boundary, inspect the serialized output, not just the source code. See Layer 8. - -8. **"Tests are simpler, they don't need full init"** (Added 2026-04) — E2E test endpoints skipping `AgentSession` registration because "it's just a test." Every entry point must do the same init steps as production. See Layer 9. - -9. **"Infrastructure code doesn't need auditing"** (Added 2026-04) — HTTP client construction, schema generation, serialization format — these are "boring plumbing" that gets skipped during audits. But they're exactly where silent failures hide (wrong TLS config via `unwrap_or_default()`, bloated schemas, missing headers). - -10. **"Some fields need fewer fallback layers"** (Added 2026-04) — A resolver function resolves model, account_id, and workspace_root from the same source chain. Model skips the DB layer because "it's always provided by the frontend." But on app restart the frontend may not have it cached. All fields in the same resolver should follow the same priority chain. See Layer 10. - -11. **"Boolean flag matches the branching need"** (Added 2026-04) — `is_channel_session` (semantic: message source) was used to branch workspace resolution (semantic: agent type). OS Agent sessions launched from the UI were `is_channel_session = false`, so they took the wrong path. When a flag drives branching, verify the flag's dimension matches the decision's dimension. - -12. **"Scatter lookups across the function"** (Added 2026-04) — `state.get_session(&id).await` called 4+ times in one function, each time to extract a different field. Each call acquires a lock, clones an Arc, and makes the function harder to reason about. Consolidate into one lookup after the point where the session is guaranteed to exist, then extract all fields at once. - -13. **"Wrap a guaranteed value in Option to match old patterns"** (Added 2026-04) — After `ok_or_else` proves a value exists, wrapping it in `Some(...)` to feed an `if let Some(ref x) = ...` downstream. This erases the compiler-enforced guarantee and forces every use-site to re-check a condition that can never be false. The downstream pattern should be updated to use the value directly. - -14. **"Pre-clone Arc fields before the closure even though the parent Arc is moved in"** (Added 2026-04) — `let provider = Arc::clone(&runtime.provider);` outside a closure, then moving both `provider` and `runtime` into the closure. Since `runtime` (an `Arc`) is moved in anyway, `Arc::clone(&runtime.provider)` can be done inside the closure, eliminating the redundant intermediate variable. - -15. **"Build a denylist by subtracting from the full set instead of building an allowlist directly"** (Added 2026-04) — Capability-derived tool availability was implemented as 4 mutable layers: (1) iterate all tools, disable those lacking capability; (2) if allowlist exists, iterate all tools again and disable those not in it; (3) apply explicit denylist; (4) hard-deny specialist tools. Plus a `META_TOOLS` in-file constant patching tools the first loop missed. The correct approach: single-pass filter over the tool catalog, producing the disabled set in one `collect()`. Conditions are AND'd (capability satisfied, not specialist, in allowlist if one exists, not in denylist). One function, one pass, no mutable accumulator, no in-file patches, no layer numbering. - ---- - -## Refactoring Planning Rules - -1. **Never declare "final" in a plan name** — there's always more. Use descriptive names. -2. **Build term overloading table FIRST** — before any plan, map every domain term to all usages. -3. **Trace full call path** — from frontend -> Tauri command -> core -> variant code. Issues hide at boundaries. -4. **Check default branches** — for every enum match, verify `_` is intentional and correct. -5. **Question "shared" modules** — if a "shared" module references specific variants, it's not truly shared. -6. **Read adjacent systems** — auditing agent definitions? Also audit providers, sessions, tools. -7. **Ask "what happens when someone adds a new variant?"** — if adding `AgentVariant::Research` breaks things silently, fix now. -8. **Sweep the class, not the instance** — when you find a bug, define its category, grep the entire scope, fix all hits. Never fix one and move on. -9. **Dump and inspect wire payloads** — for any code that sends data to an external service, serialize and inspect the actual output. Source code is not enough. -10. **Compare all entry points** — build a matrix of (entry point) x (init steps). Missing cells are bugs. -11. **Check resolver symmetry** — when a function resolves N fields from the same source chain, build a (field) x (source) matrix. Every field should check every source. Asymmetry is a latent bug. -12. **Match flag dimension to decision dimension** — when a boolean flag drives an `if/else`, ask: "does this flag's semantic axis match the decision being made?" `is_channel` (message source) branching on workspace path (agent type) is a dimension mismatch. -13. **Consolidate repeated lookups** — when `state.get_session(&id).await` (or any map/lock lookup) appears N times in one function, consolidate into one lookup and extract all needed fields. Each extra lookup is a wasted lock acquisition and a readability tax. -14. **Eliminate guaranteed-Some Option wrappers** — when a value is produced by an `ok_or` / `ok_or_else` (guaranteed non-None), do NOT wrap it in `Option` just to match a legacy `if let Some(ref x)` pattern downstream. The `Option` wrapper erases the guarantee and forces defensive code throughout. -15. **Prefer single-pass set derivation over multi-layer mutation** — when building a set of items to include/exclude, write a single `.iter().filter().collect()` with all conditions in the filter predicate. Do NOT build a mutable set and add/remove across multiple passes/layers. The single-pass version is easier to read, harder to break, and eliminates the need for in-file constant patches when the tool catalog evolves. -16. **Count sources of truth for "is the queue allowed to flush?"** — before finalizing any queue or lifecycle design, list every atom, boolean, and condition that the dispatcher checks before deciding to send. If the count is >1, reduce to exactly 1 by introducing a single FSM `phase` field. Every other signal becomes a UI mirror or a FSM input, not a decision gate. -17. **Add generation counters to every async start/stop protocol** — any time a turn, task, or job can start and stop multiple times in a session, and signals can arrive asynchronously, add a monotonically increasing integer generation to every start call. All terminal signals must carry the generation they belong to, and the handler must discard signals whose generation does not match. -18. **Audit cancel APIs for postcondition symmetry before implementation** — before writing a cancel function, list its postconditions (draft restore? mark interrupted? poison next context?). If two callers have different postconditions, the function must accept an intent parameter or be split into two functions. Never rely on frontend call timing or flag-reset order to differentiate cancel semantics. -19. **Delete shadow boolean atoms that replicate FSM phase** — when a boolean like `holdForStop` or `isRunning` is added "for safety" alongside an FSM, it almost always duplicates an FSM phase. Find the phase it corresponds to, route writers through the FSM transition, and delete the boolean. Having both guarantees they will diverge under race conditions. -20. **Trace every event handler that directly sets runtime status** — for every handler that writes `isRunning`, `runtimeStatus`, or equivalent "turn active" atoms in response to a provider event, ask: "what happens if this event arrives late, out of order, or not at all?" If the answer is "the UI freezes" or "a new turn is reset to idle," route it through the FSM with generation-checking and deadman timers instead. -21. **Enumerate all send-path call sites before shipping a queue** — before a queue dispatch system is considered complete, grep every call to the backend transport layer (`dispatchMessageBySessionType`, `sendMessage`, etc.). If >1 call site can fire for the same logical user action, there is a duplicate path. All UI controls must signal intent to the queue state machine; only the dispatcher calls the transport. -22. **Split multi-purpose atoms before they compound** — any atom whose name uses a conjunction (e.g. `userInitiatedCancelAtom` doing "mark stop episode open" AND "gate draft restoration") will cause cross-concern bleed when either concern needs to be cleared independently. At design time, name each concern separately and write one atom per concern. -23. **Name fields by their purpose, not their mechanism** — `disabled_tools` / `allowed_tools` describe the _mechanism_ (deny/allow) but not the _intent_ (user exclusion delta / subagent strict subset). Use `excluded_tools` ("tools the user/definition removed from the default set") and `restrict_to_tools` ("if non-empty, only these tools are available"). A new developer should be able to read the field name and understand _why_ the list exists without reading the surrounding code. -24. **Separate per-turn data from app-level infrastructure in request structs** — if a "per-request" struct contains fields that every single caller sets to the same app-level singleton value, those fields belong on a higher-scoped parameter (e.g. a separate `app_handle` argument) — not on the per-request struct. The struct should only contain data that genuinely varies per invocation. When app-level resources are needed inside the callee, derive them from the infrastructure handle via small extractors. -25. **Eliminate derivable constructor parameters** — when a constructor parameter is a pure function of other parameters already being passed, compute it inside the constructor body. External derivation adds maintenance burden and risks divergence when the logic is updated in one call site but not others. -26. **Audit config struct field cohesion** — for every config struct, ask: "do all fields describe the same concern?" If embedding settings, sub-agent toggles, and LLM overrides coexist in one struct, it needs splitting. One struct = one domain. Name each domain explicitly; find the right owner in the architecture (global config, per-agent definition, session record). -27. **Background subsystems must not call session-startup resolvers** — `ResolvedAgent::resolve()`, session init, and similar startup paths often enforce invariants (model present, account configured) that are valid at session-start but not in background/offline contexts. Background jobs (reflection, consolidation, cleanup) should have their own lightweight config accessors that read only what they need. -28. **Verify fallback paths for the same failure mode as the primary** — before writing `primary().unwrap_or_else(|| fallback().expect("always works"))`, ask: "can `fallback()` fail for the same reason `primary()` failed?" If the answer is yes, the `.expect()` is a latent panic. The fallback must handle the failure case, not assume it cannot occur. - -22. **"Keep a deprecated snapshot field for convenience"** (Added 2026-04) — `SessionRuntime.project_path: Option` was a snapshot set at initialization; the live source of truth was `workspace_state: Arc>` (updated by `/add-dir`, `/rm-dir`). The snapshot was never updated, so any mutation made it stale. Four consumers read the snapshot; all could read `workspace_state.read().tool_cwd()` instead. When a mutable, canonical data source exists (e.g. `workspace_state`), do NOT also store a snapshot of the same data on the same struct. The snapshot will inevitably drift, and every reader faces a hidden correctness choice between the two sources. - -23. **"Clone an owned value into a struct constructor instead of moving it"** (Added 2026-04) — `resolved: resolved.clone()` when `resolved` is an owned `ResolvedAgent` that is never used after the constructor. The `.clone()` on a large struct (with nested `Vec`, `HashMap`, `Arc` fields) wastes CPU and memory for zero benefit. Before writing `.clone()`, check: is the variable used after this point? If not, move it. Same applies to any owned value being assembled into a struct: `integrations.clone()`, `overrides.clone()`, `log_prefix.clone()` — if it's the last use site, move it. - -24. **"Pass a bare path where a structured workspace type exists"** (Added 2026-04) — `SessionRuntimeRequest` accepted `workspace_path: PathBuf`. The callee immediately wrapped it in `SessionWorkspace::new(path)` to get the structured workspace type with `project_root`, `original_cwd`, and `additional_directories`. Worse, a SECOND `SessionWorkspace::new(path.clone())` was constructed elsewhere in the same function for `AgentToolConfig`, creating two independent workspace objects from the same path — a split-brain if `additional_directories` ever differed. Fix: rename to `project_root: PathBuf` (semantically precise for what the callee receives) and use the canonical `Arc>` returned by the factory for all downstream consumers. When a structured type exists for a concept, pass it (or its constituent fields with precise names), not a bare primitive that forces the receiver to re-derive the structure. - -25. **"Relay struct that mirrors the destination struct field-for-field"** (Added 2026-04) — `SessionRuntimeRequest` (28 fields) existed only to ferry values from `init.rs` to `build_session_runtime`, which immediately unpacked 20 of them into `ToolDeps` (the struct tool constructors actually consume). Three fields (`mode_switch_manager`, `max_tokens`, `temperature`) were packed into the request but **never read** by the callee — dead code. The struct added no abstraction, no validation, no transformation; it was pure mechanical relay with a `.clone()` tax on every field transition. Fix: the caller (`init.rs`) constructs `ToolDeps` directly and passes it to the factory alongside the small set of factory-specific params (`model`, `account_id`, `disabled_tools`, `disabled_mcp_servers`, `policy_config`, `log_prefix`). **Detection rule:** when > 60 % of a struct's fields appear verbatim (same name, same type, zero transformation) in the destination struct, the relay struct is overhead. Delete it and let the caller assemble the destination struct directly. - -26. **"Copy session-level fields into a per-turn request struct via .with\_\*() chains"** (Added 2026-04) — `message.rs` called `UnifiedRequest::new()` then `apply_standard_config()` + 15 `.with_*()` builder calls to copy fields from `SessionRuntime` → `UnifiedRequest` every turn. The same fields (model, max*tokens, temperature, skills, workspace_state, etc.) were set identically for every turn in the same session. Fix: `UnifiedRequest::from_runtime(&SessionRuntime)` constructor reads all session-level fields from the runtime in a SINGLE place; callers only add per-turn fields (`mode`, `images`, `cancel_flag`, etc.). **Detection rule:** when > 50 % of a builder's `.with*\*()`calls are setting values that come from a single source struct and never change across calls, the builder needs a`from_source()`constructor. **Corollary — workspace split-brain:**`process_message`constructed a NEW`SessionWorkspace::new(path)`from`request.project_path`+ DB hydration even though`request.workspace_state`already held the identical, fully-hydrated workspace from`init.rs`. Fix: prefer `workspace_state.read().clone()` when available; fall back to DB reconstruction only for callers without a workspace_state (gateway path). - -27. **"Pass app-level singletons through per-turn input structs"** (Added 2026-04) — `TurnInput` carried infrastructure handles (`app_handle`, `lsp_manager`, `screenshot_store`, `project_path`) that were app-level singletons or derivable from `SessionRuntime`. Every caller had to extract these from `AgentAppState` and pack them into `TurnInput`; `process_message` then unpacked them to build `EventHandlerConfig` and `UnifiedMessageProcessor`. The singletons never varied per-turn — they were constant for the app's lifetime. Fix: `process_message` accepts `app_handle: Option` as a separate parameter and derives infrastructure handles internally via small extractors (`extract_lsp_manager`, `extract_screenshot_store`). `TurnInput` is reduced to only per-turn variable data (content, mode, images, ide_context, is_resume, channel, chat_id). **Detection rule:** if a per-turn/per-request struct field is set to the same value by every single caller and the value comes from a global/app-level source, it belongs on a higher-scoped parameter — not on the per-turn struct. **Corollary — inconsistent fallback:** when `TurnInput.screenshot_store` was `None`, `process_message` created a NEW empty `ScreenshotStore` instead of using the global one from `AgentAppState`. The extractor pattern fixes this: `extract_screenshot_store` always returns `AgentAppState.screenshot_store` when available, falling back to an empty store only when no app handle exists (test/public endpoints). - -28. **"Pass a derived value as a constructor parameter when the constructor already holds the source"** (Added 2026-04) — `process_message` computed `agent_id = runtime.agent_definition_id.unwrap_or(session.id)` then passed it to `UnifiedMessageProcessor::new(runtime, session, ..., agent_id, ...)`. The processor already held both `runtime` and `session` as fields, so `agent_id` was fully derivable inside the constructor. Passing it externally added a parameter, created a maintenance burden (callers must remember the derivation logic), and risked divergence if the logic were updated in one place but not the other. Fix: compute `agent_id` inside the constructor body from the already-available `runtime` and `session` fields. **Detection rule:** when a constructor parameter is a pure function of other parameters already being passed to the same constructor, eliminate it and compute inside. - -29. **"Grep-alive = alive" — reference counting is not a dead code audit** (Added 2026-04) — `UnifiedSession` (253 lines, 10 builder methods) appeared "alive" because `grep` found 15+ hits — definition, re-export chain (`types/session.rs` → `types/mod.rs` → `session/mod.rs`), `from_session()` / `to_session()` conversion methods, and a test calling those methods. But tracing from actual business entry points (Tauri commands, gateway handlers) revealed that **no production code path ever constructed or consumed `UnifiedSession`**. The runtime used `AgentSession` + `SessionRuntime`; the DB layer used `UnifiedSessionRecord` directly. The entire type hierarchy existed only to service a single round-trip test. **Detection rule:** when auditing whether a type is alive, trace from business entry points forward — not from the type's definition outward. A type that only appears in its own definition file, re-export chains, internal conversion methods, and tests is dead regardless of grep hit count. Corollary: **"aspirational abstraction"** — builder methods (`with_label`, `with_channel`, `with_project_path`) with zero callers are a strong signal the type was designed for a future that never materialized. - -30. **"Same name, different struct" — cross-module naming collision** (Added 2026-04) — Two `SessionFilter` structs existed in different modules with completely different fields: one in `types/filter.rs` (6 fields: `type_name`, `status`, `channel`, `project_path_prefix`, `limit`, `offset`) for DB queries, and another in `unified_stats/types.rs` (9 fields: `category`, `status`, `key_source`, `repo_path`, `text_query`, `sort_by`, `sort_order`, `limit`, `offset`) for frontend API. In `aggregation.rs`, both appeared in the same file distinguished only by full path (`crate::agent_core::session::SessionFilter` vs local `SessionFilter`). A new developer cannot tell which is which without reading both definitions. **Fix:** rename to make the domain explicit — `SessionListFilter` (persistence layer) vs `SessionFilter` (frontend API). **Detection rule:** after any audit, grep every exported type name across the entire codebase. If the same name appears in >1 module with different field sets, rename the narrower-scoped one. - -31. **"Overloaded config struct — one name, three unrelated domains"** (Added 2026-04) — `MemorySearchConfig` simultaneously held: (a) embedding engine settings (`provider`, `model`, `max_chunks`, `chunk_size`), (b) per-agent L3 learnings policy (`learnings_enabled`, `extract_memories_enabled`, `auto_dream_enabled`), and (c) a consolidation model override (`consolidation_model_override`). The name suggested "memory retrieval tuning" but the struct actually controlled write-path sub-agents and LLM selection. The mismatch meant every new developer had to read the full struct to understand what each field actually did. **Fix:** split by domain — `EmbeddingConfig` (app-global embedding engine, lives in `IntegrationsConfig`) + `AgentLearningsConfig` (per-agent write-path policy, lives in `AgentDefinition`). **Detection rule:** when a config struct's fields span multiple distinct subsystems (storage, LLM selection, sub-agent toggles), it is doing too many jobs. Name each concern, find the right owner, split. - -32. **"Background subsystem over-couples to full session resolver"** (Added 2026-04) — `reflection.rs` and `active_learning.rs` both called `ResolvedAgent::resolve()` just to read one boolean flag (`learnings_enabled`). But `ResolvedAgent::resolve()` enforces a strict invariant: `selected_model_id` must be present. Background subsystems run after a session ends, at which point the in-memory session is gone and the agent definition on disk may not have a model configured (OS agent never has one). Result: `reflection-pipeline` E2E silently failed with `builtin:sde has no selected_model_id`. **Fix:** introduce a lightweight `resolve_learnings_for(agent_id)` helper that uses `resolver::resolve_definition` (no model requirement) to extract only config flags. Background subsystems should call this, not `ResolvedAgent::resolve()`. **Detection rule:** any background/offline subsystem (consolidation, reflection, cleanup jobs) that calls `ResolvedAgent::resolve()` is a red flag — ask "does this code path actually need a model?" If not, use the definition resolver instead. - -33. **"expect() on a fallback path that can never succeed"** (Added 2026-04) — `GET /agent/config` called `ResolvedAgent::resolve()` on the live OS agent definition, and on failure called `.expect("fallback os_agent must resolve")` on the compiled-in `os_agent()` builtin. But `os_agent()` has `selected_model_id: None` — the fallback had the same failure mode as the primary path. The `.expect()` was a guaranteed panic disguised as defensive code. **Fix:** the fallback must use a code path that actually handles the missing-model case — in this case `AgentRuntimeView::from_definition()` which reads `learnings` and `embedding` without requiring a model. **Detection rule:** whenever you see `X.unwrap_or_else(|| Y.expect("Y must work"))`, ask "can Y actually fail for the same reason X failed?" If yes, the fallback is a lie. - -34. **"Session-layer decision misplaced in agent config layer"** (Added 2026-04) — `MemorySearchConfig.consolidation_model_override` let agent definitions override which LLM model consolidation uses. But consolidation processes learnings from a specific past session, and that session already recorded the exact model and account used at runtime (`agent_sessions.model`, `agent_sessions.account_id`). The agent-config override could diverge from what the session actually used, creating inconsistency. More importantly: the session layer already owns this decision. **Fix:** delete the override field; consolidation always reads model/account from the session record. **Detection rule:** when a background job processes per-session data, its LLM/resource selection should come from the session record — not from the agent definition. The definition controls "how the agent behaves during a live session"; the session record captures "what was actually used." Post-session processing belongs to the session record's data. - -35. **"Broadcast-only success path"** (Added 2026-05) — A Rust turn completed successfully and token usage proved the provider returned text, but the rendered chat showed only user events because the assistant message was only emitted through a transient `agent:streaming_complete` broadcast path. If the frontend listener misses, filters, or fails to upsert that broadcast, runtime truth and UI truth diverge. **Fix:** completed assistant output must be pushed to the authoritative EventStore from the runtime side; broadcasts may remain as notifications, but not as the sole persistence/visibility path. **Detection rule:** whenever an event changes durable UI history, trace whether the backend writes the canonical store directly or only emits a notification that another layer may or may not consume. - -36. **"Control action has two send/cancel dispatchers"** (Added 2026-05) — Force Send appeared to work, but `ChatView` had a duplicate direct append + send path while `useQueueDispatch` already owned queued dispatch semantics. Double paths drifted on status, model selection, queue removal, cancel timing, and error handling. **Fix:** visible queue controls should only promote/cancel/request dispatch; exactly one dispatcher owns append, dequeue, model resolution, send, and error handling. **Detection rule:** for every control button, trace from click to side effect; more than one mutation path means split ownership. (See planning rule #27.) - -37. **"Cancel flag has one meaning for Stop and Force Send"** (Added 2026-05) — User Stop and programmatic Force Send both called cancel, but their semantics differ: Stop should restore/rollback and may mark the next turn as interrupted; Force Send should cut the current turn without poisoning the follow-up turn or leaking `[Request interrupted by user]`. Sharing one cancel flag/default branch caused the next Force Send turn to be cancelled immediately or to inherit synthetic interruption text. **Fix:** cancel APIs must carry intent (`user stop` vs `programmatic interrupt`) and runtime code must branch explicitly. **Detection rule:** when two controls share a cancel path, compare postconditions; differing postconditions require separate entry points. (See planning rule #24.) - -38. **"Local row/card assertions equal orchestration success"** (Added 2026-05) — Agent Org tests proved task rows, inbox rows, and overview cards existed, but a real run still stopped at 4/6 tasks with all sessions terminal and `agent_org_runs.status = running`. The assertions were local implementation milestones, not final product outcomes. **Fix:** every orchestration audit must define final user outcome, durable run/session/task invariants, rendered UI evidence, runtime path evidence, and anti-false-positive checks before implementation. **Detection rule:** if a test would pass with `running` run + terminal sessions + open/ownerless tasks, it is not an orchestration scenario test. - -39. **"One status dimension explains the whole UI"** (Added 2026-05) — Task completion, member session liveness, member activity/intervention, and run finality are different dimensions. Mixing them makes a session look crossed-out/completed while the run says running, or hides abandoned work behind completed styling. **Fix:** store and render each dimension separately, and reconcile run finality from durable session/task state before projecting UI. **Detection rule:** every Agent Org/queue UI row should expose/assert the specific dimension it displays (`run.status`, `sessionRuntime.status`, `task.status`, owner/member activity), not infer one from another. - -40. **"Single-agent claim semantics copied into a multi-agent runtime"** (Added 2026-05) — A task tool can safely infer "current agent owns this" in a single-agent todo list, but the same inference is wrong for a coordinator in a multi-agent org. `status=in_progress` without `owner` means different things depending on caller role: a member may self-claim; a coordinator must assign a member or leave work pending. **Fix:** make task-tool semantics role-aware at the tool boundary, before store invariants fire. Recoverable misuse should return structured guidance, not an execution failure. **Detection rule:** when a tool mutates shared multi-actor state, audit caller identity (`coordinator`, `member_id`, `agent_id`, session id) before copying behavior from a single-actor system. - -41. **"Live task-board context hidden in stable prompt cache"** (Added 2026-05) — A prompt section can look correct in a dump but be stale during the next turn if it contains live DB state while marked `StableUntilClear`. For Agent Org, a stale task snapshot causes duplicate `task_create`, invalid `task_update`, and model confusion even when the store is correct. **Fix:** task board / inbox / member activity prompt sections must be `Volatile` or revision-keyed and tested through the real prompt cache policy matrix. **Detection rule:** any prompt text containing persisted runtime state must answer: what revision invalidates this, and does the cache policy enforce it? - -42. **"Clean UI while trajectory leaks tool errors"** (Added 2026-05) — Rendered UI can pass while the hidden tool trajectory contains `Error executing task_*`, SQL constraint errors, or store invariant failures. The user still sees broken agent behavior because the model consumed those errors. **Fix:** orchestration E2E must assert negative trajectory patterns alongside positive UI and DB evidence. Common model-correction paths should return JSON guidance fields (`guidance`, `already_exists`, `status_ignored`, etc.) rather than exception-shaped text. **Detection rule:** after fixing a tool error, add an E2E assertion that the previous error string cannot appear in task-tool results or live conversation trajectory. - -43. **"Pre-user schema change disguised as migration"** (Added 2026-05) — During the no-external-user stage, adding `ALTER TABLE`, legacy-table rebuilds, compatibility tests, or migration shims for a schema change creates dead compatibility logic and hides the real source of truth. **Fix:** update the canonical `CREATE TABLE` / initialization DDL directly, reset the affected local and isolated E2E databases, and verify the fresh schema path. Only write a real migration when the user explicitly says existing persisted data must be preserved. **Detection rule:** if a diff adds `ALTER TABLE`, `PRAGMA table_info` compatibility probes, `legacy` schema rebuild code, or tests named around old-table migration, stop and ask whether persisted-user compatibility is actually required. - -44. **"Debug helper marks state, test expects rendered product history"** (Added 2026-05) — A rendered Agent Org test called a debug drain endpoint that used a throwaway session, marked inbox rows read, and returned rendered messages, then expected the real coordinator chat history to contain those messages. The helper proved helper-side state, not the production caller path. **Fix:** rendered E2E must drive the same production action a user would trigger (`send_message_impl`, button click, wake/resume path) before asserting chat history. Debug endpoints may seed prerequisites or inspect state, but cannot be the mutation path for the rendered behavior under test. **Detection rule:** if a test calls `/test/*`, `debug*`, or helper-isolation endpoints and then asserts visible chat/cards changed, trace whether the endpoint writes the same EventStore/session/UI state as production; if not, the test is invalid. - -45. **"Control/sentinel records registered as user-actionable UI state"** (Added 2026-05) — File review polling treated `redo:rewind` snapshots as normal pending review entries. That re-registered a control snapshot into the pending-review registry and cleared the actual Redo anchor, disabling Redo All immediately after Undo. Similar leaks show up when protocol envelopes like `` become visible user text. **Fix:** every UI registry that consumes durable records must define an explicit inclusion predicate and exclude sentinel/control records (`redo:*`, internal batch envelopes, synthetic markers) unless the surface is an intentional diagnostic viewer. **Detection rule:** whenever a backend introduces a sentinel/prefix/control row, grep every generic list/registry consumer and add positive inclusion or explicit exclusion before shipping. - -46. **"New helper added without call-site sweep"** (Added 2026-05) — A `setTextarea` E2E helper existed, but the Agent Org description field still used `setInput`, so the spec failed with `input-missing`. Adding the helper did not update semantically matching call sites. **Fix:** introducing a specialized helper requires a sweep of all selectors/components with the same DOM/runtime shape and updating call sites in the same change. **Detection rule:** if a helper name narrows a mechanism (`setTextarea`, `selectOptionBySelector`, `drainInboxForFixture`), grep for nearby generic helper calls (`setInput`, raw debug drain, generic click) against matching test IDs/components before declaring the E2E fixed. - -47. **"Split-brain turn finality — multiple independent 'is the turn done?' signals"** (Added 2026-06) — The queue had four independent "is the session idle?" signals: `runtimeStatus` (derived from provider events), a separate `isRunning` boolean, `queueFlushRequestAtom` heuristics, and `holdSessionQueueForStopAtom`. Each defined "done" slightly differently. When they disagreed, the queue either refused to flush (frozen UI where the composer stuck in Stop state permanently) or flushed too early (next queued message sent before the previous turn fully closed, causing duplicate messages in the transcript). Any signal that can independently say "idle" is a split-brain candidate. **Fix:** introduce one canonical FSM (`turnLifecycle.ts`) with a single `phase` field. All other atoms (`runtimeStatus`, `isRunning`) become UI mirrors — they are derived from FSM transitions and cannot independently change the decision of whether to flush. The queue dispatcher reads exactly one gate: `phase === "idle"`. **Detection rule:** count the atoms/booleans your queue dispatcher reads before deciding to send — any count >1 is split-brain; reduce to one named source. - -48. **"No generation counter — stale signals from old turns poison new turns"** (Added 2026-06) — When a turn ended and a new one began immediately (e.g. Force Send into a queued follow-up), late-arriving terminal signals from the OLD turn (stream-end events that crossed the network after the new turn was already `dispatching`) would reset the NEW turn's FSM to `idle`, unlocking the queue prematurely. The FSM had no way to distinguish "signal for the current turn" from "signal for a past turn." This caused the queue to flush a third message while the second turn was still initializing. **Fix:** every `beginTurnDispatch` bumps a monotonically increasing `generation` integer synchronously (before the first `await`). Every terminal signal carries the generation it belongs to. In `markTurnTerminal`, if `signal.generation !== state.generation`, the signal is discarded silently. `forceTurnIdle` (rewind, deadman) also bumps generation to invalidate any in-flight terminal. **Detection rule:** any async start/stop system where signals can arrive out of order must carry a generation counter on every terminal signal; a terminal handler that does not check which episode it belongs to is a latent stale-signal bug. - -49. **"Dispatcher duplicates FSM logic — multiple atoms consulted for send-vs-queue"** (Added 2026-06) — The message dispatcher (the function that decides "send now or enqueue?") read `runtimeStatus`, `holdSessionQueueForStopAtom`, `queueFlushRequestAtom`, and a timing heuristic to reconstruct a rough picture of whether a turn was active. This was the FSM logic — just scattered across four atoms instead of centralized. When the FSM added a new phase (`stopping`), the dispatcher didn't know about it and kept flushing during stop. When `holdForStop` was set, the dispatcher blocked even when `runtimeStatus` was already `idle`, creating a 1–2 second frozen window. **Fix:** the dispatcher reads one field: `getTurnPhase(sessionId)`. If `"idle"` → send directly; otherwise → enqueue. All phase semantics live inside the FSM; the dispatcher has no conditional logic about what "not idle" means. **Detection rule:** grep every atom/boolean read inside the dispatch-vs-queue branch — if >1, the dispatcher is reimplementing FSM state; expose a single `getTurnPhase()` query instead. - -50. **"Cancel semantics conflated — user Stop and programmatic Force Send share the same cancel path"** (Added 2026-06) — User Stop (which should: cancel the current turn, restore the draft text, mark the follow-up as a post-stop dispatch) and programmatic Force Send (which should: cut the current turn cleanly so the queued message can send, without poisoning the next turn with interruption text) both called the same `cancelSession()` function with the same flags. The shared path set `userInitiatedCancelAtom = true`, which then caused draft restoration to fire on Force Send too. Worse, the backend received a generic cancel that prepended `[Request interrupted by user]` synthetic text to both the stopped turn and the next turn's context — causing Force Send to appear in the transcript as "user pressed Stop." **Fix:** expose two distinct cancel APIs: `stopSession()` (user intent, restores draft, marks `userInitiatedCancelAtom`) and `interruptSession()` (programmatic, no draft restoration, no poisoning of next context). The backend cancel command must carry intent too so it knows whether to prepend synthetic interruption text. **Detection rule:** if two controls share a cancel function and have differing postconditions (draft restore, interruption text, priority marking), they need separate API entry points — not a shared path plus timing differences. - -51. **"Separate 'hold' atom duplicating FSM state — two simultaneous sources of truth for queue flush"** (Added 2026-06) — `holdSessionQueueForStopAtom` was an independent boolean that said "don't flush the queue even when `runtimeStatus` is idle." It was set on Stop and cleared after a delay. This created a window where `turnPhase = "idle"` AND `holdForStop = true` simultaneously — the FSM said "go ahead", the hold atom said "wait." The queue checked both, but the clearing of `holdForStop` was on a timeout rather than tied to the actual terminal signal. Result: queue flush was delayed by the timeout even when the provider had already delivered the terminal and the FSM was idle. **Fix:** delete `holdSessionQueueForStopAtom`. The FSM `stopping` phase is the hold — when Stop is pressed, transition to `stopping`; when the terminal arrives, transition to `idle`. The queue only looks at `phase`. No separate boolean, no timeout-based clearing. **Detection rule:** for every boolean that says "don't do X even when another condition says yes," ask whether an FSM phase already represents that hold — if so, the boolean is a shadow copy and should be deleted. - -52. **"Turn finality derived from unreliable provider events instead of FSM"** (Added 2026-06) — Provider events (stream-end, `tool_call_complete`, `error`) were the sole signal driving `runtimeStatus` directly: an event handler listened on a WebSocket channel and set `isRunning = false` upon receiving any of these. But provider events are unreliable: they can arrive out of order (a `stream_complete` event arriving after a `error` from the same turn), arrive late (after a new turn has already started), or not arrive at all (network drop, backend crash). When they didn't arrive, the UI stayed frozen in "running" state until a manual refresh. **Fix:** provider events are FSM *inputs* that call `markTurnTerminal` or `markTurnRunning`. The FSM decides whether to act on them (generation check, phase guard). The UI reflects the FSM `phase`, not the raw events. Deadman timers on `dispatching` and `stopping` phases provide a hard upper bound on freeze time when events drop. **Detection rule:** grep for event handlers that directly set `isRunning`, `runtimeStatus`, or equivalent turn-active atoms without going through the FSM — each is a late/out-of-order signal vulnerability. - -53. **"Duplicate send paths in UI components — same message can be appended and dispatched twice"** (Added 2026-06) — `ChatView` had a direct `appendAndSend()` shortcut for Force Send (promoting a queued item to immediate dispatch). `useSubmitMessage` independently had its own append + dispatch path (the normal submit flow). When Force Send triggered, both paths could fire in the same React event batch: `ChatView` called `appendAndSend` directly, and the queue dispatcher also dequeued and called `dispatchMessageBySessionType`. The message appeared twice in the transcript with two different event IDs. **Fix:** exactly one dispatcher owns append + dequeue + model-resolution + dispatch + error handling. Visible queue controls (Force Send button, reorder handles) signal *intent* to the queue state machine (promote priority, mark `requiresExplicitDispatch`). The single dispatcher observes the change and executes. No UI component directly calls the transport layer. **Detection rule:** trace every backend-send call site — if >1 can fire for the same user action, there is a duplicate path; route all through the single dispatcher. - -54. **"Multi-purpose cancel atom causing cross-concern bleed"** (Added 2026-06) — `userInitiatedCancelAtom` carried two unrelated concerns: (1) "the user pressed Stop — any submit during this episode is a post-Stop explicit dispatch that gets `priority: now`" and (2) "the Stop episode is open — gate draft restoration until the terminal arrives." These two concerns were both gated on the same atom. When Force Send programmatically cleared the cancel (by calling `interruptSession` without setting `userInitiatedCancelAtom`), the draft-restoration logic didn't fire. But when the cancel path *did* set `userInitiatedCancelAtom = true` (user pressed Stop normally), draft restoration and priority dispatch both shared state — so clearing the atom for dispatch priority also prematurely closed the draft-restoration window. **Fix:** name each concern explicitly. `userInitiatedCancelAtom` → renamed to `postStopDispatchEpisodeAtom` (concern: "next submit is a post-Stop explicit dispatch"). Draft restoration is driven by `lastUserMessageAtom` + a separate `stopDraftRestorationPendingAtom` that is only set by actual user-Stop, not by programmatic interrupts. Each concern is cleared independently. **Detection rule:** when an atom name contains a conjunction (e.g. "cancel AND restore"), it carries two concerns — split it; grep all set/clear sites to confirm each concern has exactly one writer. +Provide: +- authoritative source and ownership boundary; +- layers covered and skipped; +- root cause or audit findings; +- sweep scope and evidence; +- verification performed; +- remaining risks and intentionally deferred work. diff --git a/.orgii/skills/architecture-audit/references/acceptance-criteria.md b/.orgii/skills/architecture-audit/references/acceptance-criteria.md new file mode 100644 index 000000000..5b78056b9 --- /dev/null +++ b/.orgii/skills/architecture-audit/references/acceptance-criteria.md @@ -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 +``` + +--- diff --git a/.orgii/skills/architecture-audit/references/audit-layers.md b/.orgii/skills/architecture-audit/references/audit-layers.md new file mode 100644 index 000000000..409737972 --- /dev/null +++ b/.orgii/skills/architecture-audit/references/audit-layers.md @@ -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. + +--- diff --git a/.orgii/skills/architecture-audit/references/failure-patterns.md b/.orgii/skills/architecture-audit/references/failure-patterns.md new file mode 100644 index 000000000..4806cecb6 --- /dev/null +++ b/.orgii/skills/architecture-audit/references/failure-patterns.md @@ -0,0 +1,137 @@ +# Architecture Failure Patterns and Planning Rules + +## Contents + +- [Anti-Patterns That Caused Missed Issues](#anti-patterns-that-caused-missed-issues) +- [Refactoring Planning Rules](#refactoring-planning-rules) + +## Anti-Patterns That Caused Missed Issues + +1. **"It compiles, ship it"** — `_ => OsPromptBuilder` compiles perfectly but gives Custom agents the wrong identity. Compilation correctness != semantic correctness. + +2. **"Not in my task scope"** — Provider naming was missed because task was "unify agents". Always expand audit to adjacent systems sharing terminology. + +3. **"Grep-and-skim"** — Searching `AgentVariant::Os` finds explicit uses but misses `_ =>` branches. Read the logic, not just pattern matches. + +4. **"Fix what's reported, not what's wrong"** — Fixing variant branches is shallow. The deeper issue (prompts fundamentally different, init 80% duplicated) requires reading full code paths. + +5. **"One more pass will catch everything"** — Same mental model finds same category of issues. Use different audit lenses (the 7 layers) to find different categories. + +6. **"Fix the one, forget the class"** (Added 2026-04) — Finding one blocking I/O site and fixing only that site. The correct response is: classify the pattern, grep the entire codebase, fix ALL instances. See "Systematic Sweep Discipline" above. + +7. **"Source looks fine, must be fine"** (Added 2026-04) — `schemars::openapi3()` looks like a perfectly reasonable API call. The bug is in the OUTPUT, not the source. For anything that crosses a network boundary, inspect the serialized output, not just the source code. See Layer 8. + +8. **"Tests are simpler, they don't need full init"** (Added 2026-04) — E2E test endpoints skipping `AgentSession` registration because "it's just a test." Every entry point must do the same init steps as production. See Layer 9. + +9. **"Infrastructure code doesn't need auditing"** (Added 2026-04) — HTTP client construction, schema generation, serialization format — these are "boring plumbing" that gets skipped during audits. But they're exactly where silent failures hide (wrong TLS config via `unwrap_or_default()`, bloated schemas, missing headers). + +10. **"Some fields need fewer fallback layers"** (Added 2026-04) — A resolver function resolves model, account_id, and workspace_root from the same source chain. Model skips the DB layer because "it's always provided by the frontend." But on app restart the frontend may not have it cached. All fields in the same resolver should follow the same priority chain. See Layer 10. + +11. **"Boolean flag matches the branching need"** (Added 2026-04) — `is_channel_session` (semantic: message source) was used to branch workspace resolution (semantic: agent type). OS Agent sessions launched from the UI were `is_channel_session = false`, so they took the wrong path. When a flag drives branching, verify the flag's dimension matches the decision's dimension. + +12. **"Scatter lookups across the function"** (Added 2026-04) — `state.get_session(&id).await` called 4+ times in one function, each time to extract a different field. Each call acquires a lock, clones an Arc, and makes the function harder to reason about. Consolidate into one lookup after the point where the session is guaranteed to exist, then extract all fields at once. + +13. **"Wrap a guaranteed value in Option to match old patterns"** (Added 2026-04) — After `ok_or_else` proves a value exists, wrapping it in `Some(...)` to feed an `if let Some(ref x) = ...` downstream. This erases the compiler-enforced guarantee and forces every use-site to re-check a condition that can never be false. The downstream pattern should be updated to use the value directly. + +14. **"Pre-clone Arc fields before the closure even though the parent Arc is moved in"** (Added 2026-04) — `let provider = Arc::clone(&runtime.provider);` outside a closure, then moving both `provider` and `runtime` into the closure. Since `runtime` (an `Arc`) is moved in anyway, `Arc::clone(&runtime.provider)` can be done inside the closure, eliminating the redundant intermediate variable. + +15. **"Build a denylist by subtracting from the full set instead of building an allowlist directly"** (Added 2026-04) — Capability-derived tool availability was implemented as 4 mutable layers: (1) iterate all tools, disable those lacking capability; (2) if allowlist exists, iterate all tools again and disable those not in it; (3) apply explicit denylist; (4) hard-deny specialist tools. Plus a `META_TOOLS` in-file constant patching tools the first loop missed. The correct approach: single-pass filter over the tool catalog, producing the disabled set in one `collect()`. Conditions are AND'd (capability satisfied, not specialist, in allowlist if one exists, not in denylist). One function, one pass, no mutable accumulator, no in-file patches, no layer numbering. + +--- + +## Refactoring Planning Rules + +1. **Never declare "final" in a plan name** — there's always more. Use descriptive names. +2. **Build term overloading table FIRST** — before any plan, map every domain term to all usages. +3. **Trace full call path** — from frontend -> Tauri command -> core -> variant code. Issues hide at boundaries. +4. **Check default branches** — for every enum match, verify `_` is intentional and correct. +5. **Question "shared" modules** — if a "shared" module references specific variants, it's not truly shared. +6. **Read adjacent systems** — auditing agent definitions? Also audit providers, sessions, tools. +7. **Ask "what happens when someone adds a new variant?"** — if adding `AgentVariant::Research` breaks things silently, fix now. +8. **Sweep the class, not the instance** — when you find a bug, define its category, grep the entire scope, fix all hits. Never fix one and move on. +9. **Dump and inspect wire payloads** — for any code that sends data to an external service, serialize and inspect the actual output. Source code is not enough. +10. **Compare all entry points** — build a matrix of (entry point) x (init steps). Missing cells are bugs. +11. **Check resolver symmetry** — when a function resolves N fields from the same source chain, build a (field) x (source) matrix. Every field should check every source. Asymmetry is a latent bug. +12. **Match flag dimension to decision dimension** — when a boolean flag drives an `if/else`, ask: "does this flag's semantic axis match the decision being made?" `is_channel` (message source) branching on workspace path (agent type) is a dimension mismatch. +13. **Consolidate repeated lookups** — when `state.get_session(&id).await` (or any map/lock lookup) appears N times in one function, consolidate into one lookup and extract all needed fields. Each extra lookup is a wasted lock acquisition and a readability tax. +14. **Eliminate guaranteed-Some Option wrappers** — when a value is produced by an `ok_or` / `ok_or_else` (guaranteed non-None), do NOT wrap it in `Option` just to match a legacy `if let Some(ref x)` pattern downstream. The `Option` wrapper erases the guarantee and forces defensive code throughout. +15. **Prefer single-pass set derivation over multi-layer mutation** — when building a set of items to include/exclude, write a single `.iter().filter().collect()` with all conditions in the filter predicate. Do NOT build a mutable set and add/remove across multiple passes/layers. The single-pass version is easier to read, harder to break, and eliminates the need for in-file constant patches when the tool catalog evolves. +16. **Count sources of truth for "is the queue allowed to flush?"** — before finalizing any queue or lifecycle design, list every atom, boolean, and condition that the dispatcher checks before deciding to send. If the count is >1, reduce to exactly 1 by introducing a single FSM `phase` field. Every other signal becomes a UI mirror or a FSM input, not a decision gate. +17. **Add generation counters to every async start/stop protocol** — any time a turn, task, or job can start and stop multiple times in a session, and signals can arrive asynchronously, add a monotonically increasing integer generation to every start call. All terminal signals must carry the generation they belong to, and the handler must discard signals whose generation does not match. +18. **Audit cancel APIs for postcondition symmetry before implementation** — before writing a cancel function, list its postconditions (draft restore? mark interrupted? poison next context?). If two callers have different postconditions, the function must accept an intent parameter or be split into two functions. Never rely on frontend call timing or flag-reset order to differentiate cancel semantics. +19. **Delete shadow boolean atoms that replicate FSM phase** — when a boolean like `holdForStop` or `isRunning` is added "for safety" alongside an FSM, it almost always duplicates an FSM phase. Find the phase it corresponds to, route writers through the FSM transition, and delete the boolean. Having both guarantees they will diverge under race conditions. +20. **Trace every event handler that directly sets runtime status** — for every handler that writes `isRunning`, `runtimeStatus`, or equivalent "turn active" atoms in response to a provider event, ask: "what happens if this event arrives late, out of order, or not at all?" If the answer is "the UI freezes" or "a new turn is reset to idle," route it through the FSM with generation-checking and deadman timers instead. +21. **Enumerate all send-path call sites before shipping a queue** — before a queue dispatch system is considered complete, grep every call to the backend transport layer (`dispatchMessageBySessionType`, `sendMessage`, etc.). If >1 call site can fire for the same logical user action, there is a duplicate path. All UI controls must signal intent to the queue state machine; only the dispatcher calls the transport. +22. **Split multi-purpose atoms before they compound** — any atom whose name uses a conjunction (e.g. `userInitiatedCancelAtom` doing "mark stop episode open" AND "gate draft restoration") will cause cross-concern bleed when either concern needs to be cleared independently. At design time, name each concern separately and write one atom per concern. +23. **Name fields by their purpose, not their mechanism** — `disabled_tools` / `allowed_tools` describe the _mechanism_ (deny/allow) but not the _intent_ (user exclusion delta / subagent strict subset). Use `excluded_tools` ("tools the user/definition removed from the default set") and `restrict_to_tools` ("if non-empty, only these tools are available"). A new developer should be able to read the field name and understand _why_ the list exists without reading the surrounding code. +24. **Separate per-turn data from app-level infrastructure in request structs** — if a "per-request" struct contains fields that every single caller sets to the same app-level singleton value, those fields belong on a higher-scoped parameter (e.g. a separate `app_handle` argument) — not on the per-request struct. The struct should only contain data that genuinely varies per invocation. When app-level resources are needed inside the callee, derive them from the infrastructure handle via small extractors. +25. **Eliminate derivable constructor parameters** — when a constructor parameter is a pure function of other parameters already being passed, compute it inside the constructor body. External derivation adds maintenance burden and risks divergence when the logic is updated in one call site but not others. +26. **Audit config struct field cohesion** — for every config struct, ask: "do all fields describe the same concern?" If embedding settings, sub-agent toggles, and LLM overrides coexist in one struct, it needs splitting. One struct = one domain. Name each domain explicitly; find the right owner in the architecture (global config, per-agent definition, session record). +27. **Background subsystems must not call session-startup resolvers** — `ResolvedAgent::resolve()`, session init, and similar startup paths often enforce invariants (model present, account configured) that are valid at session-start but not in background/offline contexts. Background jobs (reflection, consolidation, cleanup) should have their own lightweight config accessors that read only what they need. +28. **Verify fallback paths for the same failure mode as the primary** — before writing `primary().unwrap_or_else(|| fallback().expect("always works"))`, ask: "can `fallback()` fail for the same reason `primary()` failed?" If the answer is yes, the `.expect()` is a latent panic. The fallback must handle the failure case, not assume it cannot occur. + +22. **"Keep a deprecated snapshot field for convenience"** (Added 2026-04) — `SessionRuntime.project_path: Option` was a snapshot set at initialization; the live source of truth was `workspace_state: Arc>` (updated by `/add-dir`, `/rm-dir`). The snapshot was never updated, so any mutation made it stale. Four consumers read the snapshot; all could read `workspace_state.read().tool_cwd()` instead. When a mutable, canonical data source exists (e.g. `workspace_state`), do NOT also store a snapshot of the same data on the same struct. The snapshot will inevitably drift, and every reader faces a hidden correctness choice between the two sources. + +23. **"Clone an owned value into a struct constructor instead of moving it"** (Added 2026-04) — `resolved: resolved.clone()` when `resolved` is an owned `ResolvedAgent` that is never used after the constructor. The `.clone()` on a large struct (with nested `Vec`, `HashMap`, `Arc` fields) wastes CPU and memory for zero benefit. Before writing `.clone()`, check: is the variable used after this point? If not, move it. Same applies to any owned value being assembled into a struct: `integrations.clone()`, `overrides.clone()`, `log_prefix.clone()` — if it's the last use site, move it. + +24. **"Pass a bare path where a structured workspace type exists"** (Added 2026-04) — `SessionRuntimeRequest` accepted `workspace_path: PathBuf`. The callee immediately wrapped it in `SessionWorkspace::new(path)` to get the structured workspace type with `project_root`, `original_cwd`, and `additional_directories`. Worse, a SECOND `SessionWorkspace::new(path.clone())` was constructed elsewhere in the same function for `AgentToolConfig`, creating two independent workspace objects from the same path — a split-brain if `additional_directories` ever differed. Fix: rename to `project_root: PathBuf` (semantically precise for what the callee receives) and use the canonical `Arc>` returned by the factory for all downstream consumers. When a structured type exists for a concept, pass it (or its constituent fields with precise names), not a bare primitive that forces the receiver to re-derive the structure. + +25. **"Relay struct that mirrors the destination struct field-for-field"** (Added 2026-04) — `SessionRuntimeRequest` (28 fields) existed only to ferry values from `init.rs` to `build_session_runtime`, which immediately unpacked 20 of them into `ToolDeps` (the struct tool constructors actually consume). Three fields (`mode_switch_manager`, `max_tokens`, `temperature`) were packed into the request but **never read** by the callee — dead code. The struct added no abstraction, no validation, no transformation; it was pure mechanical relay with a `.clone()` tax on every field transition. Fix: the caller (`init.rs`) constructs `ToolDeps` directly and passes it to the factory alongside the small set of factory-specific params (`model`, `account_id`, `disabled_tools`, `disabled_mcp_servers`, `policy_config`, `log_prefix`). **Detection rule:** when > 60 % of a struct's fields appear verbatim (same name, same type, zero transformation) in the destination struct, the relay struct is overhead. Delete it and let the caller assemble the destination struct directly. + +26. **"Copy session-level fields into a per-turn request struct via .with\_\*() chains"** (Added 2026-04) — `message.rs` called `UnifiedRequest::new()` then `apply_standard_config()` + 15 `.with_*()` builder calls to copy fields from `SessionRuntime` → `UnifiedRequest` every turn. The same fields (model, max*tokens, temperature, skills, workspace_state, etc.) were set identically for every turn in the same session. Fix: `UnifiedRequest::from_runtime(&SessionRuntime)` constructor reads all session-level fields from the runtime in a SINGLE place; callers only add per-turn fields (`mode`, `images`, `cancel_flag`, etc.). **Detection rule:** when > 50 % of a builder's `.with*\*()`calls are setting values that come from a single source struct and never change across calls, the builder needs a`from_source()`constructor. **Corollary — workspace split-brain:**`process_message`constructed a NEW`SessionWorkspace::new(path)`from`request.project_path`+ DB hydration even though`request.workspace_state`already held the identical, fully-hydrated workspace from`init.rs`. Fix: prefer `workspace_state.read().clone()` when available; fall back to DB reconstruction only for callers without a workspace_state (gateway path). + +27. **"Pass app-level singletons through per-turn input structs"** (Added 2026-04) — `TurnInput` carried infrastructure handles (`app_handle`, `lsp_manager`, `screenshot_store`, `project_path`) that were app-level singletons or derivable from `SessionRuntime`. Every caller had to extract these from `AgentAppState` and pack them into `TurnInput`; `process_message` then unpacked them to build `EventHandlerConfig` and `UnifiedMessageProcessor`. The singletons never varied per-turn — they were constant for the app's lifetime. Fix: `process_message` accepts `app_handle: Option` as a separate parameter and derives infrastructure handles internally via small extractors (`extract_lsp_manager`, `extract_screenshot_store`). `TurnInput` is reduced to only per-turn variable data (content, mode, images, ide_context, is_resume, channel, chat_id). **Detection rule:** if a per-turn/per-request struct field is set to the same value by every single caller and the value comes from a global/app-level source, it belongs on a higher-scoped parameter — not on the per-turn struct. **Corollary — inconsistent fallback:** when `TurnInput.screenshot_store` was `None`, `process_message` created a NEW empty `ScreenshotStore` instead of using the global one from `AgentAppState`. The extractor pattern fixes this: `extract_screenshot_store` always returns `AgentAppState.screenshot_store` when available, falling back to an empty store only when no app handle exists (test/public endpoints). + +28. **"Pass a derived value as a constructor parameter when the constructor already holds the source"** (Added 2026-04) — `process_message` computed `agent_id = runtime.agent_definition_id.unwrap_or(session.id)` then passed it to `UnifiedMessageProcessor::new(runtime, session, ..., agent_id, ...)`. The processor already held both `runtime` and `session` as fields, so `agent_id` was fully derivable inside the constructor. Passing it externally added a parameter, created a maintenance burden (callers must remember the derivation logic), and risked divergence if the logic were updated in one place but not the other. Fix: compute `agent_id` inside the constructor body from the already-available `runtime` and `session` fields. **Detection rule:** when a constructor parameter is a pure function of other parameters already being passed to the same constructor, eliminate it and compute inside. + +29. **"Grep-alive = alive" — reference counting is not a dead code audit** (Added 2026-04) — `UnifiedSession` (253 lines, 10 builder methods) appeared "alive" because `grep` found 15+ hits — definition, re-export chain (`types/session.rs` → `types/mod.rs` → `session/mod.rs`), `from_session()` / `to_session()` conversion methods, and a test calling those methods. But tracing from actual business entry points (Tauri commands, gateway handlers) revealed that **no production code path ever constructed or consumed `UnifiedSession`**. The runtime used `AgentSession` + `SessionRuntime`; the DB layer used `UnifiedSessionRecord` directly. The entire type hierarchy existed only to service a single round-trip test. **Detection rule:** when auditing whether a type is alive, trace from business entry points forward — not from the type's definition outward. A type that only appears in its own definition file, re-export chains, internal conversion methods, and tests is dead regardless of grep hit count. Corollary: **"aspirational abstraction"** — builder methods (`with_label`, `with_channel`, `with_project_path`) with zero callers are a strong signal the type was designed for a future that never materialized. + +30. **"Same name, different struct" — cross-module naming collision** (Added 2026-04) — Two `SessionFilter` structs existed in different modules with completely different fields: one in `types/filter.rs` (6 fields: `type_name`, `status`, `channel`, `project_path_prefix`, `limit`, `offset`) for DB queries, and another in `unified_stats/types.rs` (9 fields: `category`, `status`, `key_source`, `repo_path`, `text_query`, `sort_by`, `sort_order`, `limit`, `offset`) for frontend API. In `aggregation.rs`, both appeared in the same file distinguished only by full path (`crate::agent_core::session::SessionFilter` vs local `SessionFilter`). A new developer cannot tell which is which without reading both definitions. **Fix:** rename to make the domain explicit — `SessionListFilter` (persistence layer) vs `SessionFilter` (frontend API). **Detection rule:** after any audit, grep every exported type name across the entire codebase. If the same name appears in >1 module with different field sets, rename the narrower-scoped one. + +31. **"Overloaded config struct — one name, three unrelated domains"** (Added 2026-04) — `MemorySearchConfig` simultaneously held: (a) embedding engine settings (`provider`, `model`, `max_chunks`, `chunk_size`), (b) per-agent L3 learnings policy (`learnings_enabled`, `extract_memories_enabled`, `auto_dream_enabled`), and (c) a consolidation model override (`consolidation_model_override`). The name suggested "memory retrieval tuning" but the struct actually controlled write-path sub-agents and LLM selection. The mismatch meant every new developer had to read the full struct to understand what each field actually did. **Fix:** split by domain — `EmbeddingConfig` (app-global embedding engine, lives in `IntegrationsConfig`) + `AgentLearningsConfig` (per-agent write-path policy, lives in `AgentDefinition`). **Detection rule:** when a config struct's fields span multiple distinct subsystems (storage, LLM selection, sub-agent toggles), it is doing too many jobs. Name each concern, find the right owner, split. + +32. **"Background subsystem over-couples to full session resolver"** (Added 2026-04) — `reflection.rs` and `active_learning.rs` both called `ResolvedAgent::resolve()` just to read one boolean flag (`learnings_enabled`). But `ResolvedAgent::resolve()` enforces a strict invariant: `selected_model_id` must be present. Background subsystems run after a session ends, at which point the in-memory session is gone and the agent definition on disk may not have a model configured (OS agent never has one). Result: `reflection-pipeline` E2E silently failed with `builtin:sde has no selected_model_id`. **Fix:** introduce a lightweight `resolve_learnings_for(agent_id)` helper that uses `resolver::resolve_definition` (no model requirement) to extract only config flags. Background subsystems should call this, not `ResolvedAgent::resolve()`. **Detection rule:** any background/offline subsystem (consolidation, reflection, cleanup jobs) that calls `ResolvedAgent::resolve()` is a red flag — ask "does this code path actually need a model?" If not, use the definition resolver instead. + +33. **"expect() on a fallback path that can never succeed"** (Added 2026-04) — `GET /agent/config` called `ResolvedAgent::resolve()` on the live OS agent definition, and on failure called `.expect("fallback os_agent must resolve")` on the compiled-in `os_agent()` builtin. But `os_agent()` has `selected_model_id: None` — the fallback had the same failure mode as the primary path. The `.expect()` was a guaranteed panic disguised as defensive code. **Fix:** the fallback must use a code path that actually handles the missing-model case — in this case `AgentRuntimeView::from_definition()` which reads `learnings` and `embedding` without requiring a model. **Detection rule:** whenever you see `X.unwrap_or_else(|| Y.expect("Y must work"))`, ask "can Y actually fail for the same reason X failed?" If yes, the fallback is a lie. + +34. **"Session-layer decision misplaced in agent config layer"** (Added 2026-04) — `MemorySearchConfig.consolidation_model_override` let agent definitions override which LLM model consolidation uses. But consolidation processes learnings from a specific past session, and that session already recorded the exact model and account used at runtime (`agent_sessions.model`, `agent_sessions.account_id`). The agent-config override could diverge from what the session actually used, creating inconsistency. More importantly: the session layer already owns this decision. **Fix:** delete the override field; consolidation always reads model/account from the session record. **Detection rule:** when a background job processes per-session data, its LLM/resource selection should come from the session record — not from the agent definition. The definition controls "how the agent behaves during a live session"; the session record captures "what was actually used." Post-session processing belongs to the session record's data. + +35. **"Broadcast-only success path"** (Added 2026-05) — A Rust turn completed successfully and token usage proved the provider returned text, but the rendered chat showed only user events because the assistant message was only emitted through a transient `agent:streaming_complete` broadcast path. If the frontend listener misses, filters, or fails to upsert that broadcast, runtime truth and UI truth diverge. **Fix:** completed assistant output must be pushed to the authoritative EventStore from the runtime side; broadcasts may remain as notifications, but not as the sole persistence/visibility path. **Detection rule:** whenever an event changes durable UI history, trace whether the backend writes the canonical store directly or only emits a notification that another layer may or may not consume. + +36. **"Control action has two send/cancel dispatchers"** (Added 2026-05) — Force Send appeared to work, but `ChatView` had a duplicate direct append + send path while `useQueueDispatch` already owned queued dispatch semantics. Double paths drifted on status, model selection, queue removal, cancel timing, and error handling. **Fix:** visible queue controls should only promote/cancel/request dispatch; exactly one dispatcher owns append, dequeue, model resolution, send, and error handling. **Detection rule:** for every control button, trace from click to side effect; more than one mutation path means split ownership. (See planning rule #27.) + +37. **"Cancel flag has one meaning for Stop and Force Send"** (Added 2026-05) — User Stop and programmatic Force Send both called cancel, but their semantics differ: Stop should restore/rollback and may mark the next turn as interrupted; Force Send should cut the current turn without poisoning the follow-up turn or leaking `[Request interrupted by user]`. Sharing one cancel flag/default branch caused the next Force Send turn to be cancelled immediately or to inherit synthetic interruption text. **Fix:** cancel APIs must carry intent (`user stop` vs `programmatic interrupt`) and runtime code must branch explicitly. **Detection rule:** when two controls share a cancel path, compare postconditions; differing postconditions require separate entry points. (See planning rule #24.) + +38. **"Local row/card assertions equal orchestration success"** (Added 2026-05) — Agent Org tests proved task rows, inbox rows, and overview cards existed, but a real run still stopped at 4/6 tasks with all sessions terminal and `agent_org_runs.status = running`. The assertions were local implementation milestones, not final product outcomes. **Fix:** every orchestration audit must define final user outcome, durable run/session/task invariants, rendered UI evidence, runtime path evidence, and anti-false-positive checks before implementation. **Detection rule:** if a test would pass with `running` run + terminal sessions + open/ownerless tasks, it is not an orchestration scenario test. + +39. **"One status dimension explains the whole UI"** (Added 2026-05) — Task completion, member session liveness, member activity/intervention, and run finality are different dimensions. Mixing them makes a session look crossed-out/completed while the run says running, or hides abandoned work behind completed styling. **Fix:** store and render each dimension separately, and reconcile run finality from durable session/task state before projecting UI. **Detection rule:** every Agent Org/queue UI row should expose/assert the specific dimension it displays (`run.status`, `sessionRuntime.status`, `task.status`, owner/member activity), not infer one from another. + +40. **"Single-agent claim semantics copied into a multi-agent runtime"** (Added 2026-05) — A task tool can safely infer "current agent owns this" in a single-agent todo list, but the same inference is wrong for a coordinator in a multi-agent org. `status=in_progress` without `owner` means different things depending on caller role: a member may self-claim; a coordinator must assign a member or leave work pending. **Fix:** make task-tool semantics role-aware at the tool boundary, before store invariants fire. Recoverable misuse should return structured guidance, not an execution failure. **Detection rule:** when a tool mutates shared multi-actor state, audit caller identity (`coordinator`, `member_id`, `agent_id`, session id) before copying behavior from a single-actor system. + +41. **"Live task-board context hidden in stable prompt cache"** (Added 2026-05) — A prompt section can look correct in a dump but be stale during the next turn if it contains live DB state while marked `StableUntilClear`. For Agent Org, a stale task snapshot causes duplicate `task_create`, invalid `task_update`, and model confusion even when the store is correct. **Fix:** task board / inbox / member activity prompt sections must be `Volatile` or revision-keyed and tested through the real prompt cache policy matrix. **Detection rule:** any prompt text containing persisted runtime state must answer: what revision invalidates this, and does the cache policy enforce it? + +42. **"Clean UI while trajectory leaks tool errors"** (Added 2026-05) — Rendered UI can pass while the hidden tool trajectory contains `Error executing task_*`, SQL constraint errors, or store invariant failures. The user still sees broken agent behavior because the model consumed those errors. **Fix:** orchestration E2E must assert negative trajectory patterns alongside positive UI and DB evidence. Common model-correction paths should return JSON guidance fields (`guidance`, `already_exists`, `status_ignored`, etc.) rather than exception-shaped text. **Detection rule:** after fixing a tool error, add an E2E assertion that the previous error string cannot appear in task-tool results or live conversation trajectory. + +43. **"Pre-user schema change disguised as migration"** (Added 2026-05) — During the no-external-user stage, adding `ALTER TABLE`, legacy-table rebuilds, compatibility tests, or migration shims for a schema change creates dead compatibility logic and hides the real source of truth. **Fix:** update the canonical `CREATE TABLE` / initialization DDL directly, reset the affected local and isolated E2E databases, and verify the fresh schema path. Only write a real migration when the user explicitly says existing persisted data must be preserved. **Detection rule:** if a diff adds `ALTER TABLE`, `PRAGMA table_info` compatibility probes, `legacy` schema rebuild code, or tests named around old-table migration, stop and ask whether persisted-user compatibility is actually required. + +44. **"Debug helper marks state, test expects rendered product history"** (Added 2026-05) — A rendered Agent Org test called a debug drain endpoint that used a throwaway session, marked inbox rows read, and returned rendered messages, then expected the real coordinator chat history to contain those messages. The helper proved helper-side state, not the production caller path. **Fix:** rendered E2E must drive the same production action a user would trigger (`send_message_impl`, button click, wake/resume path) before asserting chat history. Debug endpoints may seed prerequisites or inspect state, but cannot be the mutation path for the rendered behavior under test. **Detection rule:** if a test calls `/test/*`, `debug*`, or helper-isolation endpoints and then asserts visible chat/cards changed, trace whether the endpoint writes the same EventStore/session/UI state as production; if not, the test is invalid. + +45. **"Control/sentinel records registered as user-actionable UI state"** (Added 2026-05) — File review polling treated `redo:rewind` snapshots as normal pending review entries. That re-registered a control snapshot into the pending-review registry and cleared the actual Redo anchor, disabling Redo All immediately after Undo. Similar leaks show up when protocol envelopes like `` become visible user text. **Fix:** every UI registry that consumes durable records must define an explicit inclusion predicate and exclude sentinel/control records (`redo:*`, internal batch envelopes, synthetic markers) unless the surface is an intentional diagnostic viewer. **Detection rule:** whenever a backend introduces a sentinel/prefix/control row, grep every generic list/registry consumer and add positive inclusion or explicit exclusion before shipping. + +46. **"New helper added without call-site sweep"** (Added 2026-05) — A `setTextarea` E2E helper existed, but the Agent Org description field still used `setInput`, so the spec failed with `input-missing`. Adding the helper did not update semantically matching call sites. **Fix:** introducing a specialized helper requires a sweep of all selectors/components with the same DOM/runtime shape and updating call sites in the same change. **Detection rule:** if a helper name narrows a mechanism (`setTextarea`, `selectOptionBySelector`, `drainInboxForFixture`), grep for nearby generic helper calls (`setInput`, raw debug drain, generic click) against matching test IDs/components before declaring the E2E fixed. + +47. **"Split-brain turn finality — multiple independent 'is the turn done?' signals"** (Added 2026-06) — The queue had four independent "is the session idle?" signals: `runtimeStatus` (derived from provider events), a separate `isRunning` boolean, `queueFlushRequestAtom` heuristics, and `holdSessionQueueForStopAtom`. Each defined "done" slightly differently. When they disagreed, the queue either refused to flush (frozen UI where the composer stuck in Stop state permanently) or flushed too early (next queued message sent before the previous turn fully closed, causing duplicate messages in the transcript). Any signal that can independently say "idle" is a split-brain candidate. **Fix:** introduce one canonical FSM (`turnLifecycle.ts`) with a single `phase` field. All other atoms (`runtimeStatus`, `isRunning`) become UI mirrors — they are derived from FSM transitions and cannot independently change the decision of whether to flush. The queue dispatcher reads exactly one gate: `phase === "idle"`. **Detection rule:** count the atoms/booleans your queue dispatcher reads before deciding to send — any count >1 is split-brain; reduce to one named source. + +48. **"No generation counter — stale signals from old turns poison new turns"** (Added 2026-06) — When a turn ended and a new one began immediately (e.g. Force Send into a queued follow-up), late-arriving terminal signals from the OLD turn (stream-end events that crossed the network after the new turn was already `dispatching`) would reset the NEW turn's FSM to `idle`, unlocking the queue prematurely. The FSM had no way to distinguish "signal for the current turn" from "signal for a past turn." This caused the queue to flush a third message while the second turn was still initializing. **Fix:** every `beginTurnDispatch` bumps a monotonically increasing `generation` integer synchronously (before the first `await`). Every terminal signal carries the generation it belongs to. In `markTurnTerminal`, if `signal.generation !== state.generation`, the signal is discarded silently. `forceTurnIdle` (rewind, deadman) also bumps generation to invalidate any in-flight terminal. **Detection rule:** any async start/stop system where signals can arrive out of order must carry a generation counter on every terminal signal; a terminal handler that does not check which episode it belongs to is a latent stale-signal bug. + +49. **"Dispatcher duplicates FSM logic — multiple atoms consulted for send-vs-queue"** (Added 2026-06) — The message dispatcher (the function that decides "send now or enqueue?") read `runtimeStatus`, `holdSessionQueueForStopAtom`, `queueFlushRequestAtom`, and a timing heuristic to reconstruct a rough picture of whether a turn was active. This was the FSM logic — just scattered across four atoms instead of centralized. When the FSM added a new phase (`stopping`), the dispatcher didn't know about it and kept flushing during stop. When `holdForStop` was set, the dispatcher blocked even when `runtimeStatus` was already `idle`, creating a 1–2 second frozen window. **Fix:** the dispatcher reads one field: `getTurnPhase(sessionId)`. If `"idle"` → send directly; otherwise → enqueue. All phase semantics live inside the FSM; the dispatcher has no conditional logic about what "not idle" means. **Detection rule:** grep every atom/boolean read inside the dispatch-vs-queue branch — if >1, the dispatcher is reimplementing FSM state; expose a single `getTurnPhase()` query instead. + +50. **"Cancel semantics conflated — user Stop and programmatic Force Send share the same cancel path"** (Added 2026-06) — User Stop (which should: cancel the current turn, restore the draft text, mark the follow-up as a post-stop dispatch) and programmatic Force Send (which should: cut the current turn cleanly so the queued message can send, without poisoning the next turn with interruption text) both called the same `cancelSession()` function with the same flags. The shared path set `userInitiatedCancelAtom = true`, which then caused draft restoration to fire on Force Send too. Worse, the backend received a generic cancel that prepended `[Request interrupted by user]` synthetic text to both the stopped turn and the next turn's context — causing Force Send to appear in the transcript as "user pressed Stop." **Fix:** expose two distinct cancel APIs: `stopSession()` (user intent, restores draft, marks `userInitiatedCancelAtom`) and `interruptSession()` (programmatic, no draft restoration, no poisoning of next context). The backend cancel command must carry intent too so it knows whether to prepend synthetic interruption text. **Detection rule:** if two controls share a cancel function and have differing postconditions (draft restore, interruption text, priority marking), they need separate API entry points — not a shared path plus timing differences. + +51. **"Separate 'hold' atom duplicating FSM state — two simultaneous sources of truth for queue flush"** (Added 2026-06) — `holdSessionQueueForStopAtom` was an independent boolean that said "don't flush the queue even when `runtimeStatus` is idle." It was set on Stop and cleared after a delay. This created a window where `turnPhase = "idle"` AND `holdForStop = true` simultaneously — the FSM said "go ahead", the hold atom said "wait." The queue checked both, but the clearing of `holdForStop` was on a timeout rather than tied to the actual terminal signal. Result: queue flush was delayed by the timeout even when the provider had already delivered the terminal and the FSM was idle. **Fix:** delete `holdSessionQueueForStopAtom`. The FSM `stopping` phase is the hold — when Stop is pressed, transition to `stopping`; when the terminal arrives, transition to `idle`. The queue only looks at `phase`. No separate boolean, no timeout-based clearing. **Detection rule:** for every boolean that says "don't do X even when another condition says yes," ask whether an FSM phase already represents that hold — if so, the boolean is a shadow copy and should be deleted. + +52. **"Turn finality derived from unreliable provider events instead of FSM"** (Added 2026-06) — Provider events (stream-end, `tool_call_complete`, `error`) were the sole signal driving `runtimeStatus` directly: an event handler listened on a WebSocket channel and set `isRunning = false` upon receiving any of these. But provider events are unreliable: they can arrive out of order (a `stream_complete` event arriving after a `error` from the same turn), arrive late (after a new turn has already started), or not arrive at all (network drop, backend crash). When they didn't arrive, the UI stayed frozen in "running" state until a manual refresh. **Fix:** provider events are FSM *inputs* that call `markTurnTerminal` or `markTurnRunning`. The FSM decides whether to act on them (generation check, phase guard). The UI reflects the FSM `phase`, not the raw events. Deadman timers on `dispatching` and `stopping` phases provide a hard upper bound on freeze time when events drop. **Detection rule:** grep for event handlers that directly set `isRunning`, `runtimeStatus`, or equivalent turn-active atoms without going through the FSM — each is a late/out-of-order signal vulnerability. + +53. **"Duplicate send paths in UI components — same message can be appended and dispatched twice"** (Added 2026-06) — `ChatView` had a direct `appendAndSend()` shortcut for Force Send (promoting a queued item to immediate dispatch). `useSubmitMessage` independently had its own append + dispatch path (the normal submit flow). When Force Send triggered, both paths could fire in the same React event batch: `ChatView` called `appendAndSend` directly, and the queue dispatcher also dequeued and called `dispatchMessageBySessionType`. The message appeared twice in the transcript with two different event IDs. **Fix:** exactly one dispatcher owns append + dequeue + model-resolution + dispatch + error handling. Visible queue controls (Force Send button, reorder handles) signal *intent* to the queue state machine (promote priority, mark `requiresExplicitDispatch`). The single dispatcher observes the change and executes. No UI component directly calls the transport layer. **Detection rule:** trace every backend-send call site — if >1 can fire for the same user action, there is a duplicate path; route all through the single dispatcher. + +54. **"Multi-purpose cancel atom causing cross-concern bleed"** (Added 2026-06) — `userInitiatedCancelAtom` carried two unrelated concerns: (1) "the user pressed Stop — any submit during this episode is a post-Stop explicit dispatch that gets `priority: now`" and (2) "the Stop episode is open — gate draft restoration until the terminal arrives." These two concerns were both gated on the same atom. When Force Send programmatically cleared the cancel (by calling `interruptSession` without setting `userInitiatedCancelAtom`), the draft-restoration logic didn't fire. But when the cancel path *did* set `userInitiatedCancelAtom = true` (user pressed Stop normally), draft restoration and priority dispatch both shared state — so clearing the atom for dispatch priority also prematurely closed the draft-restoration window. **Fix:** name each concern explicitly. `userInitiatedCancelAtom` → renamed to `postStopDispatchEpisodeAtom` (concern: "next submit is a post-Stop explicit dispatch"). Draft restoration is driven by `lastUserMessageAtom` + a separate `stopDraftRestorationPendingAtom` that is only set by actual user-Stop, not by programmatic interrupts. Each concern is cleared independently. **Detection rule:** when an atom name contains a conjunction (e.g. "cancel AND restore"), it carries two concerns — split it; grep all set/clear sites to confirm each concern has exactly one writer. diff --git a/.orgii/skills/architecture-audit/references/planning-and-execution.md b/.orgii/skills/architecture-audit/references/planning-and-execution.md new file mode 100644 index 000000000..9aadb6894 --- /dev/null +++ b/.orgii/skills/architecture-audit/references/planning-and-execution.md @@ -0,0 +1,87 @@ +# Architecture Refactor Planning and Execution + +## Plan Structure + +### Phase ordering rules + +1. **Delete dead code first** (Phase 1 always) — reduces noise for all subsequent phases +2. **Unify duplicated logic next** — establishes shared foundations +3. **Structural/naming cleanup last** — cosmetic changes on a clean codebase + +### Phase granularity + +Each phase must be: + +- **Independently verifiable**: `cargo check` passes after each phase +- **Scope-bounded**: affects at most ~20 files +- **Both-sides**: if a Rust change affects frontend types, the frontend change is in the SAME phase + +### Plan anti-patterns + +- "Create abstraction" without "Wire it in" — creates dead code. Every "create" must have "integrate" + "delete old" in same phase. +- Phase marked "complete" without verification — each phase ends with `cargo check --all-targets` + zero warnings. +- Auditing one layer (Rust) but not the other (TypeScript) — audit both together for shared concepts. +- "Future" or "deferred" items — if worth noting, worth doing now or explicitly descoping with user. +- "It compiles, ship it" — compilation says nothing about semantic correctness. +- "Not in my task scope" — always expand audit scope to adjacent systems that share terminology. + +--- + +## Execution Discipline + +### Before each phase + +1. Verify starting state: `cargo check` passes, note warning count +2. Read the files you're about to change (never edit blind) + +### After each phase + +1. `cargo check` — zero errors +2. Warning count must be <= previous (ideally decreasing) +3. For frontend: `tsc --noEmit` or equivalent + +### Global verification (after all phases) + +Run every checklist item. If any fails, the refactor is not complete. + +--- + +## Common Refactoring Patterns + +### Unifying duplicate initialization + +When two code paths do overlapping work: + +1. List every step each path performs (side by side) +2. Mark shared steps vs variant-specific steps +3. Create factory function for shared steps, returns "base" result +4. Each variant calls factory, adds variant-specific work +5. Delete duplicated code from each variant + +### Eliminating dead abstractions + +1. Confirm zero callers (grep + compiler warnings) +2. If abstraction SHOULD be used: integrate it properly +3. If not: delete entirely +4. Never leave "aspirational" code + +### Replacing hardcoded strings with typed constants + +1. Define enum/const in ONE canonical location +2. Add `as_str()` for serialization boundaries +3. Replace ALL occurrences (including tests and comments) +4. Verify zero remaining with grep + +### Introducing an FSM to replace scattered boolean/atom state + +When "is the system in state X?" is answered by reading multiple atoms: + +1. List every atom/boolean that contributes to the answer +2. Define the complete set of mutually-exclusive states (phases) as an enum/union type +3. Write transition functions for each edge (e.g. `beginTurn`, `markRunning`, `markTerminal`, `forceIdle`) +4. Add a monotonically increasing `generation` field; bump it synchronously in every `begin*` transition +5. All signal handlers check `signal.generation === current.generation` before acting +6. Delete the old atoms; derive any needed UI booleans from the FSM phase +7. Verify: grep the codebase for the old atom names — zero remaining reads outside the FSM module + +--- diff --git a/.orgii/skills/architecture-audit/references/systematic-sweeps.md b/.orgii/skills/architecture-audit/references/systematic-sweeps.md new file mode 100644 index 000000000..c84eda7c1 --- /dev/null +++ b/.orgii/skills/architecture-audit/references/systematic-sweeps.md @@ -0,0 +1,86 @@ +# Systematic Sweep Discipline + +## Systematic Sweep Discipline (Added 2026-04) + +**When you find one instance of a problem category, you MUST sweep the entire codebase for all instances before moving on.** + +This was the single biggest failure mode in the 2026-04 audit cycle: fixing one `blocking I/O` site but not scanning for all others, fixing one `error swallowing` pattern but only in JSON/serde contexts. + +### The Rule + +For every issue found: + +1. **Classify it** — what is the general pattern? (e.g., "sync I/O in async fn", "unwrap_or_default hiding errors", "hardcoded string instead of const") +2. **Write a grep pattern** that catches ALL instances of this class, not just the one you found +3. **Run the grep across the entire target scope** (e.g., all of `agent_core/`) +4. **Record the full hit list** before fixing any +5. **Fix ALL instances** or explicitly defer with user agreement + +### Common sweep patterns + +```bash +# Blocking I/O in async context +rg "std::fs::" --type rust -l # then check if callers are async + +# Error-swallowing unwrap_or_default +rg "unwrap_or_default\(\)" --type rust + +# HTTP client construction hiding errors +rg "\.build\(\)\.unwrap_or" --type rust + +# Hardcoded finish_reason strings +rg '"stop"|"tool_calls"|"end_turn"' --type rust + +# Schema generators that may add unwanted fields +rg "SchemaSettings|into_root_schema" --type rust + +# Repeated state lookups in one function (consolidation candidate) +rg "get_session\(&session_id\)" --type rust -c # >1 per file = suspect + +# Guaranteed-Some Option wrappers (ok_or followed by Some()) +rg "ok_or.*\?\s*;" --type rust # then check if result is wrapped in Some() + +# Non-atomic multi-step DB writes (split-brain window) +rg "update_status|upsert_session" --type rust # multiple calls in sequence = candidate for merge + +# DEPRECATED fields still being assigned or read — remove or migrate first +rg -i "deprecated" --type rust -C 3 # then check: is the deprecated item still assigned/read? + +# Types alive only in definition + re-export chains (zombie types) +# For each pub struct: count callers outside its own file + mod.rs re-exports + tests +# If all hits are definition/re-export/test → dead + +# Cross-module naming collisions +# Export every pub struct name, sort, find duplicates across modules +rg "^pub struct " --type rust -l # list files, then grep struct names across all +``` + +### TypeScript/JavaScript sweep patterns + +```bash +# TypeScript: atoms serving multiple concerns +rg "Atom\b" --type ts -l # list files, then check each atom name for conjunctions + +# TypeScript: event handlers directly setting runtime status +rg "setRuntimeStatus|setIsRunning|isRunning\s*=" --type ts + +# TypeScript: duplicate send paths (direct transport calls outside dispatcher) +rg "dispatchMessage|sendMessage" --type ts -l # >1 file calling transport = suspect + +# TypeScript: UI components importing transport/dispatch directly +rg "from.*dispatcher|from.*transport" --type ts # should only appear in the dispatcher file + +# TypeScript: atoms reset in multiple places for different concerns +rg "set\(.*Atom.*false\)" --type ts # find atoms cleared in multiple locations +``` + +### Anti-pattern: "Fix the one, forget the class" + +``` +Round 1: Found blocking I/O in memory/commands.rs. Fixed it. Declared "blocking I/O: done." +Round 2: Found blocking I/O in init_helpers.rs, channel.rs, prompt_sections.rs, prompt_helpers.rs. + +Why? Because round 1 only fixed the reported instance, never swept for the pattern. +``` + +--- diff --git a/.orgii/skills/dual-instance-verification/SKILL.md b/.orgii/skills/dual-instance-verification/SKILL.md index 04eb1e491..d06d546a9 100644 --- a/.orgii/skills/dual-instance-verification/SKILL.md +++ b/.orgii/skills/dual-instance-verification/SKILL.md @@ -1,15 +1,11 @@ --- name: dual-instance-verification -description: Dual-instance (双机) real-machine verification protocol for ORG2 cloud sync and session sharing. Use before declaring any sharing/sync/collab feature or fix "verified": share/unshare, push/retract, fork/import, comments, member-floor, replay, continuation, or anything touching Org2CloudSyncEngine, collab engines, or the session channel pipeline. Also use when a sharing bug escaped earlier testing, to check which discipline below was skipped. +description: Dual-instance real-machine verification protocol for ORG2 cloud sync and session sharing. Use before declaring sharing, sync, collaboration, share/unshare, push/retract, fork/import, comments, member-floor, replay, continuation, Org2CloudSyncEngine, collaboration engine, or session-channel changes verified; also use to investigate sharing defects that escaped prior testing. --- -# Dual-Instance Verification (双机实测) +# Dual-Instance Verification -Real-machine verification of session sharing across ORG2 (primary, Neonforge) and -ORG2 Instance 2 (VantaNode). Born from a four-bug escape on 2026-07-24 where every -bug passed the old three-piece check (resource curves + feature signals + -WARN/ERROR delta). The disciplines below exist because each one, applied that day, -would have caught at least one escaped bug. +Verify session sharing across the primary ORG2 instance, the secondary instance, and the authoritative cloud rows. ## Core principle @@ -19,32 +15,16 @@ instance, and the cloud rows — and every state mutation in between is explaina ## Non-negotiables -1. **Cloud ground-truth ledger — fleet-wide, invariant-based.** Snapshot - `cloud_sessions` (session_id, deleted_at, access_mode, events_count, - events_frozen_seq, events_epoch, stored_bytes) BEFORE and AFTER every - scenario, via service key — for EVERY org the instances can see, not just the - org under test. Diff must be explainable line-by-line, and "explainable" - means a verified mechanism, not a plausible story ("that session is active" - is a story; "its rollout grew by N lines, here they are" is a mechanism). - On top of the diff, assert invariants: for every session the scenario did - NOT deliberately touch, `events_epoch` is CONSTANT and `events_count` is - monotone; flag any row with `events_epoch` above a small threshold (>3) - anywhere in the fleet. Any unexplained `deleted_at`, access_mode downgrade, - events_count drop, or epoch bump is a FAILURE even if the UI looks fine. - (Would have caught: vanished-sweep mass retract, boot out-of-scope retract, - and the #608 rewrite storm — a 28-epoch counter sat in this column for weeks - while diffs on the test org alone stayed clean.) - -2. **Destructive-effect audit at INFO level — classify by effect, not verb.** - After each scenario AND after each app boot, grep both instances' - frontend+backend logs for - `retract|untag|drop|delete|demote|evict|vanish|superseded|epoch rewrite|rewrite` - at ALL levels, not just WARN/ERROR. Every hit needs a justification. The - audit's unit is "anything that replaces or deletes cloud bytes" — a full - epoch rewrite re-uploads and REPLACES the entire stored copy and is more - destructive than a retract, yet it wears routine INFO wording and matches no - scary verb. When new log lines gain the power to mutate cloud rows, add them - to this list in the same PR. +1. **Cloud ground-truth ledger — fleet-wide and invariant-based.** Snapshot + `cloud_sessions` for every visible org before and after each scenario. Explain + every delta mechanically; for untouched sessions, require constant + `events_epoch` and monotone `events_count`. Any unexplained delete, access + downgrade, count drop, or epoch bump is a failure even when the UI looks fine. + +2. **Destructive-effect audit at INFO level.** After each scenario and app boot, + grep both instances' frontend/backend logs for destructive effects, including + retract/delete/demotion and epoch rewrites. Classify by effect rather than + severity or wording, and justify every hit. 3. **Lifecycle-boundary cells are mandatory.** The matrix is feature × lifecycle-event, not feature × instance. For every sharing feature, run at @@ -87,164 +67,16 @@ instance, and the cloud rows — and every state mutation in between is explaina `routeSessionChannelEvent`, `handleEvent(_disposed)`, and the runtime-status gate all dropped silently. Those five now log; keep that bar for new code. -8. **Resource three-piece stays — and anomalies are defects until mechanized.** +8. **Resource three-piece stays — anomalies require a mechanism.** Preserve cmd+5/Activity Monitor curves (idle ≈0%, RSS returns to baseline), feature - signals, and WARN/ERROR delta with each new line triaged. This skill ADDS to - it; it does not replace it. A recurring resource anomaly (a CPU wave on - every boot, RSS that climbs per pass) must open an investigation cell — it - may NOT be closed with a narrative. The #608 storm's boot-time CPU wave was - observed, named "ingest re-hash convergence", and normalized; the re-hash - WAS the bug. Naming an anomaly is not explaining it. - -## Invariant & determinism cells (mandatory additions per run) - -Born from the 2026-07-30 reflection on why #608 (rewrite storm), the hollow -wipe, and the scope flap all survived multiple live rounds: the protocol -asserted presence (the tested flow works) while these bugs were silent surplus -actions in the background, invisible to every existing cell. - -- **Two-boot determinism cell.** With local state unchanged, cold-boot the - instance twice and let sync passes run. Boot 2 must produce ZERO epoch - rewrites and zero destructive-effect hits (boot 1 may re-anchor once after a - legitimate format/order change — each such rewrite must be explained as - exactly-once). Nondeterminism bugs are per-process (HashMap iteration order, - random seeds); a single boot cannot sample them by construction. Measured - cost: ~18 minutes wall clock, mostly unattended. -- **Absence needs liveness beside it.** Any "zero X since the fix" claim must - pair the constant (epoch, deleted_at) with a mover (events_count, - updated_at, pass counters in the log) proving the engine actually ran over - the rows in question. Deferred/skipped sessions (e.g. scope-guard deferrals) - are UNCOVERED, not passing. -- **At least one fault-injection cell per run.** Healthy instances sample only - the happy path; the hollow wipe (empty local read while cursor covers >0) - and the scope flap (transient GitHub identity failure) lived exclusively in - degraded states no healthy-path cell can reach. Rotate through: rename/move - a local source DB mid-run (hollow read), block the identity endpoint - (lookup failure), kill the app mid-transfer (partial persist). The guard - under test must defer/refuse — any destructive act under injected fault is - a failure. -- **Unexplained delta becomes a cell.** The first ledger delta, log line, - resource pattern, or store-vs-UI discrepancy without a mechanism-level - explanation is promoted to a scenario in the CURRENT run — not noted for - later, and not handed to a background task. "Background sync noise" is the - phrase that hid a data-destroying storm; "pre-existing behavior" is the - phrase that hides everything the current PR did not happen to cause. -- **Baseline A/B answers attribution, not existence.** Reproducing a symptom on - the develop baseline proves the PR under test did not CAUSE it. It proves - nothing about whether it is a bug, and it is not a disposition. Write the two - conclusions on separate lines — attribution (this PR / not this PR) and - verdict (defect / expected, with the mechanism) — and never let the first - supply the second. A symptom that survives A/B is either explained - mechanically in this run or recorded as an OPEN DEFECT with its evidence in - the delivery message; deferring it to a chip is the same escape as "noted for - later" above. Corollary signature: **the local store holds N rows and the UI - renders 0** is always a defect — chase it to the command and the filter that - ate the rows before moving on, because the two ends disagreeing is itself the - mechanism-level question. (2026-08-01: instance-2's sidebar OLDER section - rendered zero imported rows while `imported_history_session_cache` held 257 - cursor_ide rows. A/B correctly cleared PRs #628/#576 of causing it; the - symptom was then downgraded to a background chip and is STILL unexplained as - of 2026-08-03, including after the unrelated #654 import-parse regression was - ruled out as its cause.) - -## Ledger commands - -Service key lives in `tests/e2e/.env` (machine-local). Snapshot: - -```bash -set -a; source tests/e2e/.env; set +a -curl -s "$E2E_CLOUD_SUPABASE_URL/rest/v1/cloud_sessions?org_id=eq.&select=session_id,deleted_at,access_mode,events_count,events_frozen_seq,stored_bytes,updated_at&order=session_id" \ - -H "apikey: $E2E_CLOUD_SERVICE_KEY" -H "Authorization: Bearer $E2E_CLOUD_SERVICE_KEY" \ - -H "Accept-Profile: org2_cloud" -``` - -Diff the before/after JSON; explain every changed row. Logs live at -`~/.orgii/logs/` and `~/.orgii-instance2/logs/` — backend files are UTC-dated and -UTC-stamped, frontend files local-stamped; sweep BOTH around the UTC midnight -rollover or the window silently truncates. - -## Failure taxonomy (what escaped and why — keep this list growing) - -- **Boot-window absence treated as authority**: empty scope mirror / rebuilding - cache / unrefreshed token read as "gone" → retract. Guard: grace period or - two-strike before any destructive act on boot-adjacent passes. -- **Continuation demotion read as deletion**: /compact demotes the old sibling; - exact-id lookups report it absent by design; sweeps must use the - superseded-inclusive lookup. -- **Defaults silently degrading shares**: a fork with no sharing-ladder entry - floors to metadata_only and nobody errors. Assert access_mode on the wire. -- **Self-healing hiding lost signals**: watchdog-forced completion masked every - lost agent:complete. Assert latency, treat watchdog as failure. -- **A guard upstream of every probe**: the subagent bridge swallowed fork - terminals before any instrumented drop point ran, so nine instrumented - builds all stayed silent. When probes disagree with the symptom, suspect the - model of WHERE the loss happens, not a missed branch — walk the call path - from its first line, not from the suspected failure. -- **Format drift on a shared field**: `Session.orgId` is a scope selector - (`cloud:`); fork/import wrote a bare uuid, silently removing every - ownership-derived affordance. When one writer of a shared field disagrees - with the rest, diff the live values across rows — the odd one out is the - bug. -- **Tests that encode the bug**: the two specs guarding the ownership stamp - asserted the bare form, comment included. Green tests are not evidence the - convention is right; check a spec's expectation against the consumers before - trusting it. -- **Fixture writes that silently failed**: a direct PATCH on a - write-hardened table (403 — governance requires the admin RPC) surfaced as - an empty response body, was read as success, and a whole debugging night ran - on the false premise that the scope existed server-side. Every mutation of - test-environment cloud state MUST be followed by a read-back of the same - row (compare `updated_at`, not just the field). A stale local mirror is not - server truth either — when a UI decision depends on mirrored state, diff - mirror vs server before blaming the consumer code. -- **The running binary lags the fix**: the "verified" build predated the - final commit of the file under test; every on-device probe for 40 minutes - exercised stale code. Before declaring an on-device verdict, compare the - bundle mtime against the fix file's mtime — an edit made after the last - build is not on the device, no matter how green the tests are. -- **A feature unreachable from the surface it was designed for**: Address - Comments' run path is fork-first BY DESIGN for imported histories, but that - composer mounts session-scope "none", and every consumer in the chain - (slash registry, submit interceptor) re-resolved the blank id and silently - no-opped. Reachability must be verified from the surface the design names. - Same family: candidate ordering picked a scope-matching org with no server - row (GitHub rename made two spellings one repo network), and the fork guard - demanded snapshot == summary while a LIVE source kept growing — equality - checks against a moving target are boot-window absence in another costume. -- **A silence that proves nothing**: "zero rewrites since the fix" was true - while the session was not being pushed at all (machine slept, ingest - follows the open view). An absence metric needs a liveness metric beside - it: assert the thing you want CONSTANT (ledger epoch) against the thing - that must still be MOVING (events_count / updated_at). Same shape as - watchdog-masked completion — silence and health look identical until you - measure both. -- **Presence oracles miss surplus actions**: every cell asserted "the thing I - did worked"; the escaped bugs were things NOBODY did — silent rewrites, - silent retracts, a silent wipe — that break no foreground flow. The fleet - ledger's invariant columns (epoch constant, count monotone) are the only - surface where surplus actions are visible at all. -- **Per-process nondeterminism is invisible to single-boot runs**: HashMap - iteration order reshuffled an unchanged transcript's flushed tail on every - boot, and positional chunk ids turned the shuffle into a fresh hash chain - each time. Within one app lifetime everything looked stable; only - boot-vs-boot comparison of push decisions could see it. (#608 root cause.) -- **"Pre-existing" used as a verdict**: a symptom reproduces on baseline, is - correctly cleared of THIS PR's authorship, and is then silently cleared of - being a bug at all — because the run's attention is scoped to the PR, and - attribution is the only question the run was asking. Nothing in the protocol - catches this: every other discipline here fires on something the run DID, - while this one fires on a verdict the run declined to reach. The tell is a - finding whose write-up names the PR it exonerates but never names a - mechanism. -- **Waiting for a pass the engine will never run**: the session plane follows - visible-org demand — an org's push/retract pass runs only while that org is - the active workspace. A fix whose cleanup rides "the next pass" looks - broken for any org you are not looking at. Per-org verification must OPEN - the org (switch the workspace to it) as the trigger, and cleanup claims - must name which orgs were actually visited. Corollary: rows pushed in an - earlier install/test cycle may have no surviving local push-state, and the - client rightly refuses to retract what it cannot prove it pushed — those - need a server-side fixture sweep, not more waiting. + signals, and WARN/ERROR deltas. A recurring CPU/RSS anomaly is a defect cell + until it is explained mechanically; naming it is not a disposition. + +## Conditional references + +- Read [ledger-commands.md](references/ledger-commands.md) when running or recording a real dual-instance scenario. +- Read [invariant-determinism.md](references/invariant-determinism.md) for every verification run; its two-boot, liveness, fault-injection, unexplained-delta, and baseline-attribution cells are mandatory. +- Read [failure-taxonomy.md](references/failure-taxonomy.md) when diagnosing an escaped defect, designing regression coverage, or checking whether the evidence repeats a known false-positive pattern. ## When NOT to use diff --git a/.orgii/skills/dual-instance-verification/references/failure-taxonomy.md b/.orgii/skills/dual-instance-verification/references/failure-taxonomy.md new file mode 100644 index 000000000..c54f23dcf --- /dev/null +++ b/.orgii/skills/dual-instance-verification/references/failure-taxonomy.md @@ -0,0 +1,75 @@ +# Dual-Instance Verification Failure Taxonomy + +## Failure taxonomy (what escaped and why — keep this list growing) + +- **Boot-window absence treated as authority**: empty scope mirror / rebuilding + cache / unrefreshed token read as "gone" → retract. Guard: grace period or + two-strike before any destructive act on boot-adjacent passes. +- **Continuation demotion read as deletion**: /compact demotes the old sibling; + exact-id lookups report it absent by design; sweeps must use the + superseded-inclusive lookup. +- **Defaults silently degrading shares**: a fork with no sharing-ladder entry + floors to metadata_only and nobody errors. Assert access_mode on the wire. +- **Self-healing hiding lost signals**: watchdog-forced completion masked every + lost agent:complete. Assert latency, treat watchdog as failure. +- **A guard upstream of every probe**: the subagent bridge swallowed fork + terminals before any instrumented drop point ran, so nine instrumented + builds all stayed silent. When probes disagree with the symptom, suspect the + model of WHERE the loss happens, not a missed branch — walk the call path + from its first line, not from the suspected failure. +- **Format drift on a shared field**: `Session.orgId` is a scope selector + (`cloud:`); fork/import wrote a bare uuid, silently removing every + ownership-derived affordance. When one writer of a shared field disagrees + with the rest, diff the live values across rows — the odd one out is the + bug. +- **Tests that encode the bug**: the two specs guarding the ownership stamp + asserted the bare form, comment included. Green tests are not evidence the + convention is right; check a spec's expectation against the consumers before + trusting it. +- **Fixture writes that silently failed**: a direct PATCH on a + write-hardened table (403 — governance requires the admin RPC) surfaced as + an empty response body, was read as success, and a whole debugging night ran + on the false premise that the scope existed server-side. Every mutation of + test-environment cloud state MUST be followed by a read-back of the same + row (compare `updated_at`, not just the field). A stale local mirror is not + server truth either — when a UI decision depends on mirrored state, diff + mirror vs server before blaming the consumer code. +- **The running binary lags the fix**: the "verified" build predated the + final commit of the file under test; every on-device probe for 40 minutes + exercised stale code. Before declaring an on-device verdict, compare the + bundle mtime against the fix file's mtime — an edit made after the last + build is not on the device, no matter how green the tests are. +- **A feature unreachable from the surface it was designed for**: Address + Comments' run path is fork-first BY DESIGN for imported histories, but that + composer mounts session-scope "none", and every consumer in the chain + (slash registry, submit interceptor) re-resolved the blank id and silently + no-opped. Reachability must be verified from the surface the design names. + Same family: candidate ordering picked a scope-matching org with no server + row (GitHub rename made two spellings one repo network), and the fork guard + demanded snapshot == summary while a LIVE source kept growing — equality + checks against a moving target are boot-window absence in another costume. +- **A silence that proves nothing**: "zero rewrites since the fix" was true + while the session was not being pushed at all (machine slept, ingest + follows the open view). An absence metric needs a liveness metric beside + it: assert the thing you want CONSTANT (ledger epoch) against the thing + that must still be MOVING (events_count / updated_at). Same shape as + watchdog-masked completion — silence and health look identical until you + measure both. +- **Presence oracles miss surplus actions**: foreground flows can pass while + background sync silently rewrites, retracts, or wipes unrelated rows. Fleet + invariants are the surface that exposes actions nobody requested. +- **Per-process nondeterminism is invisible to single-boot runs**: unordered + iteration can produce a different hash chain after every restart while one + process stays stable. Compare two cold boots with unchanged local state. +- **"Pre-existing" used as a verdict**: baseline reproduction answers whether + the current PR caused a symptom; it does not decide whether the symptom is a + defect. Record attribution and mechanism-level verdict separately. +- **Waiting for a pass the engine will never run**: the session plane follows + visible-org demand — an org's push/retract pass runs only while that org is + the active workspace. A fix whose cleanup rides "the next pass" looks + broken for any org you are not looking at. Per-org verification must OPEN + the org (switch the workspace to it) as the trigger, and cleanup claims + must name which orgs were actually visited. Corollary: rows pushed in an + earlier install/test cycle may have no surviving local push-state, and the + client rightly refuses to retract what it cannot prove it pushed — those + need a server-side fixture sweep, not more waiting. diff --git a/.orgii/skills/dual-instance-verification/references/invariant-determinism.md b/.orgii/skills/dual-instance-verification/references/invariant-determinism.md new file mode 100644 index 000000000..53ee42e85 --- /dev/null +++ b/.orgii/skills/dual-instance-verification/references/invariant-determinism.md @@ -0,0 +1,39 @@ +# Invariant and Determinism Cells + +These cells are mandatory for every dual-instance verification run. They catch +silent surplus actions that presence-only foreground checks cannot see. + +## Two-boot determinism + +With local state unchanged, cold-boot the instance twice and let sync passes run. +Boot 2 must produce zero epoch rewrites and zero destructive-effect hits. Boot 1 +may re-anchor once after a legitimate format/order change, but every rewrite must +be explained as exactly-once. + +## Absence requires liveness + +Pair every "zero X" claim with a mover proving the engine ran over the rows in +question: `events_count`, `updated_at`, or an explicit pass counter. Deferred or +skipped sessions are uncovered, not passing. + +## Fault injection + +Run at least one degraded-state cell per verification. Rotate among moving a +local source database mid-read, blocking identity lookup, and terminating the app +mid-transfer. The system must defer or refuse destructive work under the fault. + +## Unexplained deltas + +Promote the first unexplained ledger delta, log line, resource pattern, or +store-versus-UI discrepancy to a scenario in the current run. Do not close it as +"background noise" or defer it without a mechanism-level verdict. + +## Baseline attribution + +Baseline A/B establishes attribution, not correctness. Record separately: + +1. whether the current PR caused the symptom; and +2. whether the symptom is a defect or expected behavior, with the mechanism. + +If the local store contains rows while the UI renders none, treat the mismatch as +a defect until the command and filter responsible are identified. diff --git a/.orgii/skills/dual-instance-verification/references/ledger-commands.md b/.orgii/skills/dual-instance-verification/references/ledger-commands.md new file mode 100644 index 000000000..96aef055f --- /dev/null +++ b/.orgii/skills/dual-instance-verification/references/ledger-commands.md @@ -0,0 +1,19 @@ +# Dual-Instance Verification Ledger Commands + +## Ledger commands + +Service key lives in `tests/e2e/.env` (machine-local). Snapshot: + +```bash +set -a; source tests/e2e/.env; set +a +curl -s "$E2E_CLOUD_SUPABASE_URL/rest/v1/cloud_sessions?select=org_id,session_id,deleted_at,access_mode,events_count,events_frozen_seq,events_epoch,stored_bytes,updated_at&order=org_id,session_id" \ + -H "apikey: $E2E_CLOUD_SERVICE_KEY" -H "Authorization: Bearer $E2E_CLOUD_SERVICE_KEY" \ + -H "Accept-Profile: org2_cloud" +``` + +Snapshot every org visible to the instances, not only the org under test. Diff +the before/after JSON; explain every changed row and assert constant +`events_epoch` plus monotone `events_count` for untouched sessions. Logs live at +`~/.orgii/logs/` and `~/.orgii-instance2/logs/` — backend files are UTC-dated and +UTC-stamped, frontend files local-stamped; sweep BOTH around the UTC midnight +rollover or the window silently truncates. diff --git a/.orgii/skills/e2e-testing/SKILL.md b/.orgii/skills/e2e-testing/SKILL.md index c04fb6d32..f3f0a4886 100644 --- a/.orgii/skills/e2e-testing/SKILL.md +++ b/.orgii/skills/e2e-testing/SKILL.md @@ -1,237 +1,30 @@ -# ORGII E2E Testing - -ORGII keeps two separate E2E surfaces. Do not use one as proof for the other. - -1. **Core UI E2E** — `tests/e2e/specs/core/`, driven by WebDriverIO against the debug-built Tauri app. -2. **Rust runtime E2E** — `src-tauri/crates/e2e-test/`, a Rust HTTP client against debug-only `/agent/test/*` endpoints. - -A Rust HTTP scenario can prove runtime state. It does not prove a user can see, click, or recover through the rendered UI. If a feature has a button, card, menu item, wizard field, status pill, or visible chat behavior, it needs rendered UI coverage. - -## Core UI E2E policy - -`tests/e2e` is the final UI regression suite. Keep it small and clean. - -Rules: - -- Extend an existing core spec before creating a new one. -- Do not put historical audits, migration sweeps, subsystem experiments, or one-off debug specs under `tests/e2e`. -- Every UI spec must perform a real rendered action or assert a real rendered result. Debug helpers may seed state, but cannot be the only proof. -- Provider capacity failures, especially Gemini 429/rate-limit/capacity errors, are infra/provider issues unless ORGII mishandles the rendered error or runtime state. -- OAuth refresh failures with permanent invalid-token messages are account health blockers. ORGII should record the failure and disable the account immediately; UI E2E should report them separately from product regressions and continue through configured account/model fallback chains when available. - -## Anti-false-prosperity policy - -A green E2E result is not accepted unless it proves the production behavior being claimed. - -- Do not mark a scenario `PASS` when the critical action is replaced by a frontend mock, synthetic success flag, debug-only responder, or helper that bypasses the production command/event path. -- Debug helpers may establish deterministic preconditions, but the user-visible action under test must still use the production click/command/dispatcher path. -- Do not use corrective follow-up prompts, extra retry prompts, or stronger second instructions to make an agent pass after the original user path failed. The first-path failure is the product signal. -- Do not count a matrix run as proof for multiple labels unless each requested label produced independent evidence. Combined fallback output is not per-label proof. -- Do not promote old green rows after prompt text, harness setup, account fallback, or product semantics changed. Rerun only the affected rows and record current-code evidence. -- For interactive cards, assert the full lifecycle: rendered reason/body, actionable button, production response command, backend/runtime state change, and final rendered state. A pill text change alone is not enough. -- For mode/tool claims, assert session-scoped effective tools (`agent_list_effective_tools_for_session`, `/agent/test/effective-tools/:session_id`, or `__e2e.listEffectiveToolsForSession`) rather than global registry or historical renderability. -- Treat provider quota/capacity blocks as `BLOCKED`, not `PASS`; never route around them silently to manufacture green coverage. - -## Real-interaction regression policy - -If a bug was found by a human using the rendered app, the regression test must replay the human interaction path closely enough to fail before the fix. - -- Prefer `browser.keys`, real clicks, focus/blur, menu navigation, and visible-state waits over `browser.execute` text injection. Direct DOM mutation is allowed only for deterministic setup, never as proof that input handling works. -- Contenteditable tests must first prove the keystroke/input actually changed the rendered editor text before asserting menus or buttons. A keydown-only signal (menu opened but `editor.textContent` stayed empty) is a harness/product bug signal, not proof that `@query` or `/query` works. -- Prefer real keyboard clearing for contenteditable surfaces (`Cmd/Ctrl+A`, delete/backspace, then `browser.keys(...)`) and wait for the rendered editor text to stabilize before menu assertions. Do not require `document.activeElement` to remain the editor after `@` or `/` opens a portal/menu; focus may legitimately move while the editor text remains the source of truth. -- If Tauri WebDriver element-click/focus is flaky for a contenteditable surface, the spec may use `document.execCommand('insertText')` plus a real bubbling `InputEvent` to exercise the product `onInput` path, but it must assert the editor text, query consumption, and final visible result. Do not use plain `textContent = ...` as the behavior under test. -- If a test helper replaces DOM text directly, it must be limited to deterministic seed/setup. A regression for inline `@`/slash/menu behavior must use keyboard input or an `InputEvent` path that can fail when React draft state restores stale text. -- A composer/menu test must assert all user-visible invariants: previous draft text preserved, transient query text consumed, inserted pill/chip visible, focus restored when product requires it, and send/stop state correct. -- Stop/Pause/Queue tests must assert immediate button state, composer interactivity, stream cessation, queue retention/non-autoflush, and draft restoration when a not-yet-sent message is canceled. -- Use seeded events only to create durable transcript preconditions. Rendering assertions must still inspect the actual chat UI, including grouped/aggregated blocks, not only `data-testid` fragments that disappear under aggregation. -- A test helper that calls `setTextarea`, `insertText`, `ensureRepoSelected`, or a debug seed path must include a comment or assertion explaining which production behavior is still being exercised afterward. -- If a prior test used a shortcut and missed a bug, update the skill/spec so future tests forbid the shortcut for that class of interaction. - -## Multi-repo workspace regression policy - -Multi-root behavior must be treated as a first-class product contract, not a display patch. - -- Distinguish durable session root, primary workspace folder, active editor folder, search result source repo, and tool-event target path. Do not use active editor focus as a durable session root unless the user explicitly selected it. -- Explicitly test that `activeFolderAtom` / active editor focus can move to a secondary repo without changing the durable launch root. Agent session launch defaults to the primary workspace folder; active folder is only a UI/current-focus concept. -- Every multi-repo UI test should include at least two repos with colliding filenames so source attribution cannot be inferred from basename alone. -- `@` search and context menus must render persistent source evidence (`repoName`/path badge), not only hover-only titles. -- File-path extraction must go through a shared extractor that handles canonical payload variants (`file_path`, `filePath`, `target_file`, `targetFile`, `path`) across backend normalization, frontend props, summaries, and grouped chat blocks. Do not add component-local `a || b || c` chains. -- Read-file rendering tests must cover single blocks, grouped `ReadFileGroup`, and aggregate `ActionSummaryGroup` summaries, including camelCase Cursor-style tool payloads. -- Multi-repo session launch tests must assert the selected durable repo path in the launch payload/runtime snapshot, and separately assert that UI search/source badges remain accurate for non-primary repos. -- Multi-root E2E setup helpers must not silently call single-repo pinning (`repoPath: E2E_REPO_PATH`, `ensureRepoSelected`, or equivalent) after seeding multiple folders. If a creator/helper needs account/model setup only, pass through the existing selected multi-root workspace and assert `workspaceFolders`/source evidence afterward. -- Multi-repo path-rendering tests must use self-contained fixture paths with colliding basenames and payload key variants (`targetFile`, `file_path`, nested `success.filePath`, etc.). Do not depend on another local checkout such as `claude_code`, and do not accept generic labels like `file` as path evidence. -- Multi-repo search tests must validate both the visible source badge and the selected path/pill value. A menu that merely contains two basenames is not enough; the chosen secondary repo result must survive click/keyboard selection into the composer context. -- When a multi-repo bug is fixed in one surface, sweep all equivalent surfaces: session creator, existing chat composer, context menu, event normalizer, props extraction, tool-call summary, grouped transcript rendering, and E2E seed helpers. -- Audit duplicate workspace state sources before adding patches. If both a canonical store path and an older/legacy workspace atom module exist, tests must import the production path and the diff must not add another derived source of truth. -- Any E2E helper that sets `activeFolder`, `selectedRepo`, `workspaceFolders`, or launch workspace fields must return a snapshot of all related atoms/paths and the test must assert the durable/active distinction immediately. If a failure message shows the target path inside the folder dump but matching failed, inspect argument marshaling and path normalization before adding fallback display logic. -- Do not fix multi-repo bugs with display-only band-aids. A valid fix names the data contract, centralizes extraction/resolution once, wires all consumers to that contract, and adds negative tests that would fail if a component-local fallback or single-repo pinning returned. - -## Matrix evidence policy - -For requested provider/runtime matrices, each row needs current-code evidence and a clear outcome. - -- Record the exact account/model/runtime row, command/spec/scenario, and result (`PASS`, `BLOCKED`, or `FAIL`). -- A fallback due to Gemini 429/capacity may satisfy the user flow only if the row records the original provider block and the fallback model that actually produced evidence. -- Do not claim “9 matrix all green” from a subset run, a prior commit, or a combined fallback. Every row must produce independent evidence or be explicitly marked `BLOCKED` with provider/account reason. -- Matrix rows should reuse deterministic fake-provider/debug bridges for product invariants and reserve live-provider rows for integration smoke, otherwise provider flakiness hides product regressions. - -## Workspace fixture policy - -Core UI E2E must not depend on `yorg_frontend`, `yoyo-evolve`, or any external local project. - -The WDIO runner creates a self-contained git fixture repo by default: - -- Path: `/tmp/orgii-e2e-workspace-repo` -- Rebuilt at runner startup -- Contains `README.md`, `package.json`, `src/math.ts`, and an initial git commit -- Safe for agent mutation tests - -Only set `E2E_REPO_PATH` when intentionally overriding with another sandbox git repo. The runner must reject explicit paths that do not exist, are not git repos, or lack the baseline files. - -Session launch specs should pass the fixture `repoPath` through the same session configure/launch caller path the user uses. Do not add a separate `before` hook that only calls `ensureRepoSelected`; that helper can time out before the app is fully settled and can mask the real launch path with WebDriver harness failures. - -Recommended isolated UI run when the developer app may already be using `1998`: - -```bash -E2E_ISOLATED_RUN=1 \ -E2E_ORGII_HOME="/tmp/orgii-e2e-home" \ -E2E_FRONTEND_PORT=21998 \ -E2E_WEBDRIVER_PORT=24444 \ -E2E_IDE_SERVER_PORT=23847 \ -npm test -``` - -WDIO managed runs must not kill or reuse a developer's active ORGII app by default. The runner should fail fast if its managed ports are occupied unless `E2E_ALLOW_PORT_CLEANUP=1` is explicitly set. When `E2E_FRONTEND_PORT` differs from `1998`, the WDIO runner must make that real by building the webdriver debug app against a temporary Tauri `devUrl` pointing at the requested port, then restoring `src-tauri/tauri.conf.json` exactly. Merely starting webpack on a non-1998 port is false isolation because an unpatched debug app still loads `http://localhost:1998`. - -## Rust runtime E2E policy - -`e2e-test` is a deterministic runtime contract suite, not a second UI suite and not a live-provider platform matrix. Keep it much smaller than the historical audit-era suite. - -Keep: +--- +name: e2e-testing +description: ORGII rendered UI and Rust runtime end-to-end testing guidance. Use when adding, repairing, reviewing, or running WebDriverIO specs under tests/e2e, Rust HTTP runtime scenarios under src-tauri/crates/e2e-test, multi-repo workspace regressions, orchestration flows, file-change or diff behavior, queue/turn lifecycle tests, or CLI session reload coverage. +--- -- Backend/runtime invariants not covered by rendered UI E2E. -- Deterministic debug-endpoint coverage for memory, learning, permissions, worktree, session recovery, housekeeping, LSP, gateway/sync/MCP contracts, subagent dispatch, and tool execution invariants. -- Tool-policy and agent-definition contracts that are hard to observe from UI alone, especially positive/negative schema or policy assertions. Use the session-scoped effective-tools surface (`agent_list_effective_tools_for_session`, `/agent/test/effective-tools/:session_id`, or `__e2e.listEffectiveToolsForSession`) rather than global `list_all_tools` or registry-only `/agent/test/tool-schemas/:session_id` when asserting what a running agent can actually see in a mode-filtered prompt. -- Scenarios with stable setup, stable assertions, and explicit teardown/isolation. - -Delete or move out: - -- Historical phase/audit scenarios whose invariant is already covered by a canonical scenario. -- Long-running live-LLM scenarios that mainly duplicate UI/platform matrix behavior. -- Provider-specific smoke tests that are better covered by core UI matrix rows. -- Memory/learning tests that only prove the model can recall rendered text; keep state/DB/policy pins instead. -- Plan lifecycle tests that assert user-visible card/button behavior; keep only backend policy/snapshot invariants in Rust. -- Scenarios whose only assertion is `HTTP 200` or loose text without a stable invariant. -- Dead helper modules/functions not registered in `main.rs` and not called by a registered scenario. - -When cleaning Rust E2E: - -1. Inspect `src-tauri/crates/e2e-test/src/main.rs` scenario registry. -2. Count groups with `cargo run -p e2e-test -- --list` or a local registry parser. -3. Remove entries only when their invariant is duplicated, obsolete, flaky by design, or moved to UI E2E. -4. Delete the module/function after removing the registry entry. -5. Run `cargo check -p e2e-test` and `cargo fmt`. - -## Choosing the right layer - -| Claim | Required coverage | -| -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------- | -| Runtime state/tool behavior is correct | Rust `e2e-test` or deterministic debug endpoint | -| Tauri command shape is correct | Command call plus TypeScript contract/type check | -| Card/button/menu/slash action works | WDIO rendered-app test | -| Configurable UI field works | Five-layer alignment plus rendered submit-path coverage | -| Unsupported feature is gone | Negative UI assertion and/or backend action-list assertion | -| Provider returns 429/capacity | classify as provider capacity unless ORGII mishandles it | -| Agent has the right tools | session-scoped effective-tools API plus backend schema/policy negative+positive test; add UI smoke only when the tools are visible | - -## Commands - -Rust runtime: - -```bash -cd src-tauri -cargo run -p e2e-test -- --list -cargo run -p e2e-test -- --scenario plan-mode-denies-writes -cargo run -p e2e-test -- --group memory -cargo check -p e2e-test -cargo fmt -p e2e-test -``` - -Core UI: - -```bash -cd tests/e2e -# Full matrix (requires E2E_ALLOW_PORT_CLEANUP=1 if dev app is running) -E2E_ALLOW_PORT_CLEANUP=1 npm test -- --spec './specs/core/session-controls-ui.spec.mjs' - -# Single scenario -E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=rewind npm test -- --spec './specs/core/session-controls-ui.spec.mjs' -E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-build-direct npm test -- --spec './specs/core/session-controls-ui.spec.mjs' -E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-update npm test -- --spec './specs/core/session-controls-ui.spec.mjs' -E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-edit-resend npm test -- --spec './specs/core/session-controls-ui.spec.mjs' - -# Core UI: -E2E_ALLOW_PORT_CLEANUP=1 npm test -- --spec './specs/core/chat-rendering-ui.spec.mjs' -``` - -## Result-driven orchestration regressions - -Rendered orchestration tests must start from the final user result and durable end-state, not from implementation breadcrumbs. A test that creates rows/cards but would still pass when a run stops with incomplete work is a false positive. - -For Agent Org / multi-member / queue scenarios, every complex spec should state or encode: - -- Final user outcome: what the org/team should have achieved. -- Final DB invariants: run status converged; all completed tasks are actually completed; open work is visibly blocked/abandoned; no `in_progress` task lacks an owner. -- Final UI evidence: resident member sessions are visible and switchable in the left sidebar, member transcripts open, and task board status does not contradict run/session state. -- Runtime path evidence: production launch, task tools, member wake/drain, inbox delivery, and member-session messaging paths ran; debug helpers only seed or inspect. -- Anti-false-positive checks: scenario-named tasks, passive inbox rows, synthetic cards, or a second corrective prompt do not count as success. -- Latest-session evidence: after a user reports a stuck or contradictory Agent Org run, inspect the newest run/session/task/inbox durable state and the latest terminal/app log before claiming the fix is verified. Do not infer success from an older green scenario or from a different synthetic run. - -A Rust runtime E2E that posts protocol messages or calls debug endpoints is not proof that Agent Org works in the app. Rendered Agent Org acceptance must drive the production launch path from the UI, wait for production wake/drain/member-session turns, and then assert both UI and durable DB finality. Unit tests and Rust E2E can pin regressions, but they cannot be used as the sole evidence for “Agent Org advances correctly.” - -Minimum failure cases that a valid orchestration spec must catch: - -- A `running` run with no active member session and no unread inbox work to wake/drain (stalled run). -- `status = "in_progress"` with `owner = null` (ownerless work persisted). -- A `completed` run that still has `pending` or `in_progress` tasks. -- A run that appears visually populated but cannot make forward progress without a corrective second prompt. - -
-Agent Org-specific failure cases - -- A `running` run with `pending` / `in_progress` tasks, no active member session, and no unread inbox work to wake/drain. -- A ready assigned `pending` task whose dependencies are all completed, but whose owner has no unread `TaskAssigned` inbox row and no active member turn. -- Unread org inbox rows that remain unread after the owner/member production session has gone idle/completed a turn. -- `status = "in_progress"` with `owner = null`, or `status = "in_progress"` set by the coordinator for another member rather than by the owning member's claim/drain path. -- A `completed` run that still has `pending` or `in_progress` tasks. -- Member sessions visible in the coordinator overview but absent from the left sidebar. -- Multiple org members sharing the same `agent_id` / `agent_definition_id` while inbox delivery, wake, drain, task owner, and task-tool authorization are only keyed by `agent_id`. -- A run that appears visually populated but cannot make forward progress from the original user prompt without a corrective second prompt. - -
- -## File changes panel and diff view +# ORGII E2E Testing -The inline file-review panel was removed in `fbf20c78`. The composer "files pill" now opens Agent Station Diff view instead of expanding an inline card. Tests that assert file-change review must account for this: +ORGII has two separate E2E surfaces: -- The files pill (`data-testid="composer-section-files"`) click calls `openAgentStationDiff`, which sets `chatPanelMaximized=false`, `stationMode="agent-station"`, `simulatorSelectedAppAtom=AppType.DIFF`, and `replayModeAtom="replay"`. -- **`chatPanelMaximized` must be false** before `ActivitySimulator` (and thus `SimulatorWorkstationTabHeader`) renders. If the chat panel is maximized, the diff view pane is suppressed and its buttons are invisible. -- Undo All button: `data-testid="file-changes-undo-all"` in `SimulatorWorkstationTabHeader` (rendered only when `pendingCount > 0`). -- Redo All button: `data-testid="file-changes-redo-all"` in `SimulatorWorkstationTabHeader` (rendered only when `redoSnapshotAnchors.length > 0`). -- The E2E helper `__e2e.openAgentStationDiff()` sets the same atoms as the product pill callback and is available as a fallback when Tauri WebDriver `element.click()` misses React synthetic events. Use `invokeE2E("openAgentStationDiff")` after failed pill-click retries. -- `waitForFileChangesPanel` in `agentQueuedWorkspaceHelpers.mjs` encapsulates this logic: it retries pill click 3×, then falls back to `invokeE2E("openAgentStationDiff")`, then waits for `[data-testid="file-changes-undo-all"]` or `[data-testid="replay-tab-diff-filter"]` to appear. -- For plan-build-direct, always call `waitForRuntimeIdle()` before `waitForFileChangesPanel()` — the Undo All button only activates after the build turn is fully idle. +1. Core UI E2E uses WebDriverIO against the debug-built Tauri app. +2. Rust runtime E2E uses an HTTP client against debug-only agent test endpoints. -## Plan, rewind, and streaming regressions +Runtime evidence never substitutes for rendered UI evidence when a user-visible control or result is involved. -Rendered plan tests must pin the caller path, not only derived UI helpers: +## Core workflow -- Rewind/edit-resend must invalidate stale queued turns and cancel the active turn before sending the replacement message. -- Plan update/edit-resend tests must assert no duplicate pending/drafting cards, only the latest plan is buildable, and stale revisions remain visible only as archived history when appropriate. -- Plan card diagnostics must distinguish surfaces by `data-plan-surface`: `transcript` cards in chat history, `current` cards in the pending review bar, and communication-side preview cards. -- Stop/Send button E2E clicks must be atomic with the expected `data-state`. -- Long-running debug HTTP endpoints must be called from the WDIO Node process, not through `browser.executeAsyncScript(fetch(...))`. -- Streaming marker assertions must wait for the full expected marker, not only for assistant text to become non-empty or change. +1. Identify whether the claim is UI-visible, runtime-only, or cross-layer. +2. Read [commands.md](references/commands.md) for layer selection and supported commands. +3. Load only the policy matching the scenario: + - Rendered interaction or visible recovery: [core-ui-policy.md](references/core-ui-policy.md) + - Multi-repo, fixtures, or account/model matrices: [workspace-and-matrix.md](references/workspace-and-matrix.md) + - Debug endpoint and runtime state: [rust-runtime-policy.md](references/rust-runtime-policy.md) + - Agent-org orchestration, diffs, plans, rewind, or streaming: [orchestration-and-diff.md](references/orchestration-and-diff.md) + - Queue, turn finality, or CLI reload: [lifecycle-and-reload.md](references/lifecycle-and-reload.md) +4. Extend the smallest existing stable scenario that proves the user outcome. +5. Capture positive end-state evidence and explicit anti-false-positive evidence. +6. Report product failures separately from provider, account, port, and environment blockers. ## Hard rules @@ -241,21 +34,3 @@ Rendered plan tests must pin the caller path, not only derived UI helpers: - Never add a rendered UI claim to Rust-only coverage. - Never add a debug endpoint that tests only a helper when the bug is in the caller path. - Never preserve an obsolete scenario just because it once caught a phase bug; keep the invariant, not the phase artifact. - -## Turn lifecycle and queue E2E - -The turn lifecycle is controlled by a FSM in `src/engines/SessionCore/control/turnLifecycle.ts`. Key concepts for E2E: - -- `turnPhase`: `"idle"` | `"dispatching"` | `"running"` | `"stopping"` — use `inspectChatState().turnPhase` to assert turn state precisely. -- `turnGeneration`: monotonically increasing counter; each new turn gets a new generation. Stale terminal signals from old turns are ignored. -- `runtimeStatus`: derived from FSM + provider signals. Use `waitForRuntimeIdle()` to wait for `runtimeStatus === "idle"` and `turnPhase === "idle"`. -- Queue tests must assert `queuedMessages` array contents, not just UI queue item count. Use `inspectChatState().queuedMessages`. -- After Stop, queued messages with `requiresExplicitDispatch=true` must not auto-flush. Assert queue retention before any follow-up send. - -## CLI session reload - -CLI sessions (claude-code, codex, cursor-cli, gemini-cli) reload history from SQLite via `cliAdapter.loadHistory` after a browser refresh. Key rules: - -- After `cli_agent_truncate_after_chunk` (edit-resend / rewind), the product code calls `deleteCachedSession` and `evictSession` to ensure a clean reload. Tests that reload after rewind must wait for `chatEventCount > 0`, not just `activeSessionId` match. -- `reloadAndOpenActiveSession` retries `openSession` up to 3× with 3s gaps — CLI adapter settling after reload is a known race that does not affect real users (who wait for UI to render before clicking). -- Do not mark CLI reload as a product failure if `chatEventCount: 0` appears only after a programmatic `browser.refresh()` + immediate `openSession`. Verify with multiple runs before treating as a stable product bug. diff --git a/.orgii/skills/e2e-testing/references/commands.md b/.orgii/skills/e2e-testing/references/commands.md new file mode 100644 index 000000000..e601db578 --- /dev/null +++ b/.orgii/skills/e2e-testing/references/commands.md @@ -0,0 +1,43 @@ +# E2E Layer Selection and Commands + +## Choosing the right layer + +| Claim | Required coverage | +| -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------- | +| Runtime state/tool behavior is correct | Rust `e2e-test` or deterministic debug endpoint | +| Tauri command shape is correct | Command call plus TypeScript contract/type check | +| Card/button/menu/slash action works | WDIO rendered-app test | +| Configurable UI field works | Five-layer alignment plus rendered submit-path coverage | +| Unsupported feature is gone | Negative UI assertion and/or backend action-list assertion | +| Provider returns 429/capacity | classify as provider capacity unless ORGII mishandles it | +| Agent has the right tools | session-scoped effective-tools API plus backend schema/policy negative+positive test; add UI smoke only when the tools are visible | + +## Commands + +Rust runtime: + +```bash +cd src-tauri +cargo run -p e2e-test -- --list +cargo run -p e2e-test -- --scenario plan-mode-denies-writes +cargo run -p e2e-test -- --group memory +cargo check -p e2e-test +cargo fmt -p e2e-test +``` + +Core UI: + +```bash +cd tests/e2e +# Full matrix (requires E2E_ALLOW_PORT_CLEANUP=1 if dev app is running) +E2E_ALLOW_PORT_CLEANUP=1 npm test -- --spec './specs/core/session-controls-ui.spec.mjs' + +# Single scenario +E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=rewind npm test -- --spec './specs/core/session-controls-ui.spec.mjs' +E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-build-direct npm test -- --spec './specs/core/session-controls-ui.spec.mjs' +E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-update npm test -- --spec './specs/core/session-controls-ui.spec.mjs' +E2E_ALLOW_PORT_CLEANUP=1 E2E_CONTROL_SCENARIOS=plan-edit-resend npm test -- --spec './specs/core/session-controls-ui.spec.mjs' + +# Core UI: +E2E_ALLOW_PORT_CLEANUP=1 npm test -- --spec './specs/core/chat-rendering-ui.spec.mjs' +``` diff --git a/.orgii/skills/e2e-testing/references/core-ui-policy.md b/.orgii/skills/e2e-testing/references/core-ui-policy.md new file mode 100644 index 000000000..114b159b9 --- /dev/null +++ b/.orgii/skills/e2e-testing/references/core-ui-policy.md @@ -0,0 +1,41 @@ +# Core UI E2E Policy + +## Core UI E2E policy + +`tests/e2e` is the final UI regression suite. Keep it small and clean. + +Rules: + +- Extend an existing core spec before creating a new one. +- Do not put historical audits, migration sweeps, subsystem experiments, or one-off debug specs under `tests/e2e`. +- Every UI spec must perform a real rendered action or assert a real rendered result. Debug helpers may seed state, but cannot be the only proof. +- Provider capacity failures, especially Gemini 429/rate-limit/capacity errors, are infra/provider issues unless ORGII mishandles the rendered error or runtime state. +- OAuth refresh failures with permanent invalid-token messages are account health blockers. ORGII should record the failure and disable the account immediately; UI E2E should report them separately from product regressions and continue through configured account/model fallback chains when available. + +## Anti-false-prosperity policy + +A green E2E result is not accepted unless it proves the production behavior being claimed. + +- Do not mark a scenario `PASS` when the critical action is replaced by a frontend mock, synthetic success flag, debug-only responder, or helper that bypasses the production command/event path. +- Debug helpers may establish deterministic preconditions, but the user-visible action under test must still use the production click/command/dispatcher path. +- Do not use corrective follow-up prompts, extra retry prompts, or stronger second instructions to make an agent pass after the original user path failed. The first-path failure is the product signal. +- Do not count a matrix run as proof for multiple labels unless each requested label produced independent evidence. Combined fallback output is not per-label proof. +- Do not promote old green rows after prompt text, harness setup, account fallback, or product semantics changed. Rerun only the affected rows and record current-code evidence. +- For interactive cards, assert the full lifecycle: rendered reason/body, actionable button, production response command, backend/runtime state change, and final rendered state. A pill text change alone is not enough. +- For mode/tool claims, assert session-scoped effective tools (`agent_list_effective_tools_for_session`, `/agent/test/effective-tools/:session_id`, or `__e2e.listEffectiveToolsForSession`) rather than global registry or historical renderability. +- Treat provider quota/capacity blocks as `BLOCKED`, not `PASS`; never route around them silently to manufacture green coverage. + +## Real-interaction regression policy + +If a bug was found by a human using the rendered app, the regression test must replay the human interaction path closely enough to fail before the fix. + +- Prefer `browser.keys`, real clicks, focus/blur, menu navigation, and visible-state waits over `browser.execute` text injection. Direct DOM mutation is allowed only for deterministic setup, never as proof that input handling works. +- Contenteditable tests must first prove the keystroke/input actually changed the rendered editor text before asserting menus or buttons. A keydown-only signal (menu opened but `editor.textContent` stayed empty) is a harness/product bug signal, not proof that `@query` or `/query` works. +- Prefer real keyboard clearing for contenteditable surfaces (`Cmd/Ctrl+A`, delete/backspace, then `browser.keys(...)`) and wait for the rendered editor text to stabilize before menu assertions. Do not require `document.activeElement` to remain the editor after `@` or `/` opens a portal/menu; focus may legitimately move while the editor text remains the source of truth. +- If Tauri WebDriver element-click/focus is flaky for a contenteditable surface, the spec may use `document.execCommand('insertText')` plus a real bubbling `InputEvent` to exercise the product `onInput` path, but it must assert the editor text, query consumption, and final visible result. Do not use plain `textContent = ...` as the behavior under test. +- If a test helper replaces DOM text directly, it must be limited to deterministic seed/setup. A regression for inline `@`/slash/menu behavior must use keyboard input or an `InputEvent` path that can fail when React draft state restores stale text. +- A composer/menu test must assert all user-visible invariants: previous draft text preserved, transient query text consumed, inserted pill/chip visible, focus restored when product requires it, and send/stop state correct. +- Stop/Pause/Queue tests must assert immediate button state, composer interactivity, stream cessation, queue retention/non-autoflush, and draft restoration when a not-yet-sent message is canceled. +- Use seeded events only to create durable transcript preconditions. Rendering assertions must still inspect the actual chat UI, including grouped/aggregated blocks, not only `data-testid` fragments that disappear under aggregation. +- A test helper that calls `setTextarea`, `insertText`, `ensureRepoSelected`, or a debug seed path must include a comment or assertion explaining which production behavior is still being exercised afterward. +- If a prior test used a shortcut and missed a bug, update the skill/spec so future tests forbid the shortcut for that class of interaction. diff --git a/.orgii/skills/e2e-testing/references/lifecycle-and-reload.md b/.orgii/skills/e2e-testing/references/lifecycle-and-reload.md new file mode 100644 index 000000000..b71d367f7 --- /dev/null +++ b/.orgii/skills/e2e-testing/references/lifecycle-and-reload.md @@ -0,0 +1,19 @@ +# Turn Lifecycle, Queue, and Reload E2E + +## Turn lifecycle and queue E2E + +The turn lifecycle is controlled by a FSM in `src/engines/SessionCore/control/turnLifecycle.ts`. Key concepts for E2E: + +- `turnPhase`: `"idle"` | `"dispatching"` | `"running"` | `"stopping"` — use `inspectChatState().turnPhase` to assert turn state precisely. +- `turnGeneration`: monotonically increasing counter; each new turn gets a new generation. Stale terminal signals from old turns are ignored. +- `runtimeStatus`: derived from FSM + provider signals. Use `waitForRuntimeIdle()` to wait for `runtimeStatus === "idle"` and `turnPhase === "idle"`. +- Queue tests must assert `queuedMessages` array contents, not just UI queue item count. Use `inspectChatState().queuedMessages`. +- After Stop, queued messages with `requiresExplicitDispatch=true` must not auto-flush. Assert queue retention before any follow-up send. + +## CLI session reload + +CLI sessions (claude-code, codex, cursor-cli, gemini-cli) reload history from SQLite via `cliAdapter.loadHistory` after a browser refresh. Key rules: + +- After `cli_agent_truncate_after_chunk` (edit-resend / rewind), the product code calls `deleteCachedSession` and `evictSession` to ensure a clean reload. Tests that reload after rewind must wait for `chatEventCount > 0`, not just `activeSessionId` match. +- `reloadAndOpenActiveSession` retries `openSession` up to 3× with 3s gaps — CLI adapter settling after reload is a known race that does not affect real users (who wait for UI to render before clicking). +- Do not mark CLI reload as a product failure if `chatEventCount: 0` appears only after a programmatic `browser.refresh()` + immediate `openSession`. Verify with multiple runs before treating as a stable product bug. diff --git a/.orgii/skills/e2e-testing/references/orchestration-and-diff.md b/.orgii/skills/e2e-testing/references/orchestration-and-diff.md new file mode 100644 index 000000000..62544c4ef --- /dev/null +++ b/.orgii/skills/e2e-testing/references/orchestration-and-diff.md @@ -0,0 +1,60 @@ +# Orchestration, File Change, and Streaming Regressions + +## Result-driven orchestration regressions + +Rendered orchestration tests must start from the final user result and durable end-state, not from implementation breadcrumbs. A test that creates rows/cards but would still pass when a run stops with incomplete work is a false positive. + +For Agent Org / multi-member / queue scenarios, every complex spec should state or encode: + +- Final user outcome: what the org/team should have achieved. +- Final DB invariants: run status converged; all completed tasks are actually completed; open work is visibly blocked/abandoned; no `in_progress` task lacks an owner. +- Final UI evidence: resident member sessions are visible and switchable in the left sidebar, member transcripts open, and task board status does not contradict run/session state. +- Runtime path evidence: production launch, task tools, member wake/drain, inbox delivery, and member-session messaging paths ran; debug helpers only seed or inspect. +- Anti-false-positive checks: scenario-named tasks, passive inbox rows, synthetic cards, or a second corrective prompt do not count as success. +- Latest-session evidence: after a user reports a stuck or contradictory Agent Org run, inspect the newest run/session/task/inbox durable state and the latest terminal/app log before claiming the fix is verified. Do not infer success from an older green scenario or from a different synthetic run. + +A Rust runtime E2E that posts protocol messages or calls debug endpoints is not proof that Agent Org works in the app. Rendered Agent Org acceptance must drive the production launch path from the UI, wait for production wake/drain/member-session turns, and then assert both UI and durable DB finality. Unit tests and Rust E2E can pin regressions, but they cannot be used as the sole evidence for “Agent Org advances correctly.” + +Minimum failure cases that a valid orchestration spec must catch: + +- A `running` run with no active member session and no unread inbox work to wake/drain (stalled run). +- `status = "in_progress"` with `owner = null` (ownerless work persisted). +- A `completed` run that still has `pending` or `in_progress` tasks. +- A run that appears visually populated but cannot make forward progress without a corrective second prompt. + +
+Agent Org-specific failure cases + +- A `running` run with `pending` / `in_progress` tasks, no active member session, and no unread inbox work to wake/drain. +- A ready assigned `pending` task whose dependencies are all completed, but whose owner has no unread `TaskAssigned` inbox row and no active member turn. +- Unread org inbox rows that remain unread after the owner/member production session has gone idle/completed a turn. +- `status = "in_progress"` with `owner = null`, or `status = "in_progress"` set by the coordinator for another member rather than by the owning member's claim/drain path. +- A `completed` run that still has `pending` or `in_progress` tasks. +- Member sessions visible in the coordinator overview but absent from the left sidebar. +- Multiple org members sharing the same `agent_id` / `agent_definition_id` while inbox delivery, wake, drain, task owner, and task-tool authorization are only keyed by `agent_id`. +- A run that appears visually populated but cannot make forward progress from the original user prompt without a corrective second prompt. + +
+ +## File changes panel and diff view + +The inline file-review panel was removed in `fbf20c78`. The composer "files pill" now opens Agent Station Diff view instead of expanding an inline card. Tests that assert file-change review must account for this: + +- The files pill (`data-testid="composer-section-files"`) click calls `openAgentStationDiff`, which sets `chatPanelMaximized=false`, `stationMode="agent-station"`, `simulatorSelectedAppAtom=AppType.DIFF`, and `replayModeAtom="replay"`. +- **`chatPanelMaximized` must be false** before `ActivitySimulator` (and thus `SimulatorWorkstationTabHeader`) renders. If the chat panel is maximized, the diff view pane is suppressed and its buttons are invisible. +- Undo All button: `data-testid="file-changes-undo-all"` in `SimulatorWorkstationTabHeader` (rendered only when `pendingCount > 0`). +- Redo All button: `data-testid="file-changes-redo-all"` in `SimulatorWorkstationTabHeader` (rendered only when `redoSnapshotAnchors.length > 0`). +- The E2E helper `__e2e.openAgentStationDiff()` sets the same atoms as the product pill callback and is available as a fallback when Tauri WebDriver `element.click()` misses React synthetic events. Use `invokeE2E("openAgentStationDiff")` after failed pill-click retries. +- `waitForFileChangesPanel` in `agentQueuedWorkspaceHelpers.mjs` encapsulates this logic: it retries pill click 3×, then falls back to `invokeE2E("openAgentStationDiff")`, then waits for `[data-testid="file-changes-undo-all"]` or `[data-testid="replay-tab-diff-filter"]` to appear. +- For plan-build-direct, always call `waitForRuntimeIdle()` before `waitForFileChangesPanel()` — the Undo All button only activates after the build turn is fully idle. + +## Plan, rewind, and streaming regressions + +Rendered plan tests must pin the caller path, not only derived UI helpers: + +- Rewind/edit-resend must invalidate stale queued turns and cancel the active turn before sending the replacement message. +- Plan update/edit-resend tests must assert no duplicate pending/drafting cards, only the latest plan is buildable, and stale revisions remain visible only as archived history when appropriate. +- Plan card diagnostics must distinguish surfaces by `data-plan-surface`: `transcript` cards in chat history, `current` cards in the pending review bar, and communication-side preview cards. +- Stop/Send button E2E clicks must be atomic with the expected `data-state`. +- Long-running debug HTTP endpoints must be called from the WDIO Node process, not through `browser.executeAsyncScript(fetch(...))`. +- Streaming marker assertions must wait for the full expected marker, not only for assistant text to become non-empty or change. diff --git a/.orgii/skills/e2e-testing/references/rust-runtime-policy.md b/.orgii/skills/e2e-testing/references/rust-runtime-policy.md new file mode 100644 index 000000000..374544179 --- /dev/null +++ b/.orgii/skills/e2e-testing/references/rust-runtime-policy.md @@ -0,0 +1,30 @@ +# Rust Runtime E2E Policy + +## Rust runtime E2E policy + +`e2e-test` is a deterministic runtime contract suite, not a second UI suite and not a live-provider platform matrix. Keep it much smaller than the historical audit-era suite. + +Keep: + +- Backend/runtime invariants not covered by rendered UI E2E. +- Deterministic debug-endpoint coverage for memory, learning, permissions, worktree, session recovery, housekeeping, LSP, gateway/sync/MCP contracts, subagent dispatch, and tool execution invariants. +- Tool-policy and agent-definition contracts that are hard to observe from UI alone, especially positive/negative schema or policy assertions. Use the session-scoped effective-tools surface (`agent_list_effective_tools_for_session`, `/agent/test/effective-tools/:session_id`, or `__e2e.listEffectiveToolsForSession`) rather than global `list_all_tools` or registry-only `/agent/test/tool-schemas/:session_id` when asserting what a running agent can actually see in a mode-filtered prompt. +- Scenarios with stable setup, stable assertions, and explicit teardown/isolation. + +Delete or move out: + +- Historical phase/audit scenarios whose invariant is already covered by a canonical scenario. +- Long-running live-LLM scenarios that mainly duplicate UI/platform matrix behavior. +- Provider-specific smoke tests that are better covered by core UI matrix rows. +- Memory/learning tests that only prove the model can recall rendered text; keep state/DB/policy pins instead. +- Plan lifecycle tests that assert user-visible card/button behavior; keep only backend policy/snapshot invariants in Rust. +- Scenarios whose only assertion is `HTTP 200` or loose text without a stable invariant. +- Dead helper modules/functions not registered in `main.rs` and not called by a registered scenario. + +When cleaning Rust E2E: + +1. Inspect `src-tauri/crates/e2e-test/src/main.rs` scenario registry. +2. Count groups with `cargo run -p e2e-test -- --list` or a local registry parser. +3. Remove entries only when their invariant is duplicated, obsolete, flaky by design, or moved to UI E2E. +4. Delete the module/function after removing the registry entry. +5. Run `cargo check -p e2e-test` and `cargo fmt`. diff --git a/.orgii/skills/e2e-testing/references/workspace-and-matrix.md b/.orgii/skills/e2e-testing/references/workspace-and-matrix.md new file mode 100644 index 000000000..f63478fb2 --- /dev/null +++ b/.orgii/skills/e2e-testing/references/workspace-and-matrix.md @@ -0,0 +1,57 @@ +# Workspace and Matrix E2E Policy + +## Multi-repo workspace regression policy + +Multi-root behavior must be treated as a first-class product contract, not a display patch. + +- Distinguish durable session root, primary workspace folder, active editor folder, search result source repo, and tool-event target path. Do not use active editor focus as a durable session root unless the user explicitly selected it. +- Explicitly test that `activeFolderAtom` / active editor focus can move to a secondary repo without changing the durable launch root. Agent session launch defaults to the primary workspace folder; active folder is only a UI/current-focus concept. +- Every multi-repo UI test should include at least two repos with colliding filenames so source attribution cannot be inferred from basename alone. +- `@` search and context menus must render persistent source evidence (`repoName`/path badge), not only hover-only titles. +- File-path extraction must go through a shared extractor that handles canonical payload variants (`file_path`, `filePath`, `target_file`, `targetFile`, `path`) across backend normalization, frontend props, summaries, and grouped chat blocks. Do not add component-local `a || b || c` chains. +- Read-file rendering tests must cover single blocks, grouped `ReadFileGroup`, and aggregate `ActionSummaryGroup` summaries, including camelCase Cursor-style tool payloads. +- Multi-repo session launch tests must assert the selected durable repo path in the launch payload/runtime snapshot, and separately assert that UI search/source badges remain accurate for non-primary repos. +- Multi-root E2E setup helpers must not silently call single-repo pinning (`repoPath: E2E_REPO_PATH`, `ensureRepoSelected`, or equivalent) after seeding multiple folders. If a creator/helper needs account/model setup only, pass through the existing selected multi-root workspace and assert `workspaceFolders`/source evidence afterward. +- Multi-repo path-rendering tests must use self-contained fixture paths with colliding basenames and payload key variants (`targetFile`, `file_path`, nested `success.filePath`, etc.). Do not depend on another local checkout such as `claude_code`, and do not accept generic labels like `file` as path evidence. +- Multi-repo search tests must validate both the visible source badge and the selected path/pill value. A menu that merely contains two basenames is not enough; the chosen secondary repo result must survive click/keyboard selection into the composer context. +- When a multi-repo bug is fixed in one surface, sweep all equivalent surfaces: session creator, existing chat composer, context menu, event normalizer, props extraction, tool-call summary, grouped transcript rendering, and E2E seed helpers. +- Audit duplicate workspace state sources before adding patches. If both a canonical store path and an older/legacy workspace atom module exist, tests must import the production path and the diff must not add another derived source of truth. +- Any E2E helper that sets `activeFolder`, `selectedRepo`, `workspaceFolders`, or launch workspace fields must return a snapshot of all related atoms/paths and the test must assert the durable/active distinction immediately. If a failure message shows the target path inside the folder dump but matching failed, inspect argument marshaling and path normalization before adding fallback display logic. +- Do not fix multi-repo bugs with display-only band-aids. A valid fix names the data contract, centralizes extraction/resolution once, wires all consumers to that contract, and adds negative tests that would fail if a component-local fallback or single-repo pinning returned. + +## Matrix evidence policy + +For requested provider/runtime matrices, each row needs current-code evidence and a clear outcome. + +- Record the exact account/model/runtime row, command/spec/scenario, and result (`PASS`, `BLOCKED`, or `FAIL`). +- A fallback due to Gemini 429/capacity may satisfy the user flow only if the row records the original provider block and the fallback model that actually produced evidence. +- Do not claim “9 matrix all green” from a subset run, a prior commit, or a combined fallback. Every row must produce independent evidence or be explicitly marked `BLOCKED` with provider/account reason. +- Matrix rows should reuse deterministic fake-provider/debug bridges for product invariants and reserve live-provider rows for integration smoke, otherwise provider flakiness hides product regressions. + +## Workspace fixture policy + +Core UI E2E must not depend on `yorg_frontend`, `yoyo-evolve`, or any external local project. + +The WDIO runner creates a self-contained git fixture repo by default: + +- Path: `/tmp/orgii-e2e-workspace-repo` +- Rebuilt at runner startup +- Contains `README.md`, `package.json`, `src/math.ts`, and an initial git commit +- Safe for agent mutation tests + +Only set `E2E_REPO_PATH` when intentionally overriding with another sandbox git repo. The runner must reject explicit paths that do not exist, are not git repos, or lack the baseline files. + +Session launch specs should pass the fixture `repoPath` through the same session configure/launch caller path the user uses. Do not add a separate `before` hook that only calls `ensureRepoSelected`; that helper can time out before the app is fully settled and can mask the real launch path with WebDriver harness failures. + +Recommended isolated UI run when the developer app may already be using `1998`: + +```bash +E2E_ISOLATED_RUN=1 \ +E2E_ORGII_HOME="/tmp/orgii-e2e-home" \ +E2E_FRONTEND_PORT=21998 \ +E2E_WEBDRIVER_PORT=24444 \ +E2E_IDE_SERVER_PORT=23847 \ +npm test +``` + +WDIO managed runs must not kill or reuse a developer's active ORGII app by default. The runner should fail fast if its managed ports are occupied unless `E2E_ALLOW_PORT_CLEANUP=1` is explicitly set. When `E2E_FRONTEND_PORT` differs from `1998`, the WDIO runner must make that real by building the webdriver debug app against a temporary Tauri `devUrl` pointing at the requested port, then restoring `src-tauri/tauri.conf.json` exactly. Merely starting webpack on a non-1998 port is false isolation because an unpatched debug app still loads `http://localhost:1998`. diff --git a/.orgii/skills/org2-performance-guard/SKILL.md b/.orgii/skills/org2-performance-guard/SKILL.md index 3cd881768..5f074e837 100644 --- a/.orgii/skills/org2-performance-guard/SKILL.md +++ b/.orgii/skills/org2-performance-guard/SKILL.md @@ -1,11 +1,11 @@ --- name: org2-performance-guard -description: Prevent CPU, RAM, I/O, and background-work regressions in ORG2. Use when adding or reviewing polling, timers, Realtime subscriptions, event listeners, workers, streaming paths, caches, pagination, external-history scans, cloud sync, source-control loading, per-session state, or multi-instance behavior; also use before delivering a performance refactor or any feature that stays alive while the UI is idle or hidden. +description: Prevent CPU, RAM, I/O, and background-work regressions in ORG2. Use when adding or reviewing polling, timers, retries, Realtime subscriptions, event listeners, workers, streaming paths, caches, pagination, external-history scans, cloud sync, source-control loading, per-session retained state, multi-instance lifecycle, or any feature that remains active while the UI is idle or hidden. --- # ORG2 Performance Guard -Apply a lifecycle-first performance audit to every changed runtime path. Preserve correctness and realtime behavior while making idle work demand-driven, shared, bounded, scoped, and disposable. +Apply a lifecycle-first audit. Preserve correctness and realtime behavior while making background work demand-driven, shared, bounded, scoped, and disposable. ## Non-negotiable invariants @@ -27,138 +27,10 @@ Require all applicable invariants before delivery: - Isolate secondary Tauri identities completely: data home, external-history home, ports, cookies/auth, and app-lifetime caches. - Keep rendered E2E strict. Missing UI must fail with diagnostics; never turn a regression into `console.warn`, catch-and-continue, or a debug-helper bypass. -## Required workflow +## Workflow -### 1. Establish the performance surface +1. Read [surface-and-lifecycle.md](references/surface-and-lifecycle.md) to inventory active/idle/hidden behavior and repeated mount, account, endpoint, org, repo, and session transitions. +2. Read [runtime-patterns.md](references/runtime-patterns.md) when selecting or reviewing polling, push invalidation, single-flight, caches, history loading, subscriptions, privacy scopes, or equivalent-path sweeps. +3. Read [verification-and-delivery.md](references/verification-and-delivery.md) before declaring the work complete. -Read the changed call chain from its production entry point. Inventory every resource the change can create or retain: - -- `setInterval`, recursive `setTimeout`, `requestAnimationFrame`, debounce, retry, backoff -- DOM/Tauri/network listeners and Realtime channels -- workers, subprocesses, watchers, file scans, git operations, database reads -- module globals, atom maps, per-store maps, promises, abort controllers, buffers -- React subscriptions, selectors, derived arrays, render-time sorting/grouping -- eager list/history/diff/replay loading - -Use targeted searches, adapting paths to the diff: - -```powershell -rg -n "setInterval|setTimeout|requestAnimationFrame|addEventListener|listen\(|subscribe|channel\(" src src-tauri -rg -n "new Map|new Set|WeakMap|cache|inFlight|buffer|queue|history" src src-tauri -rg -n "poll|refresh|retry|scan|watch|stream|delta|dispose|cleanup|abort" src src-tauri -``` - -Do not treat grep hits as findings. Trace ownership, start conditions, steady-state behavior, and cleanup. - -### 2. Build the lifecycle matrix - -For each resource, record the required behavior in these states: - -| Dimension | States to check | -| --------- | --------------------------------------------------------------- | -| App | start, idle, active, shutdown | -| Document | visible, hidden, focus return | -| Network | online, offline, retry/backoff | -| Identity | signed out, signed in, refresh, account switch, endpoint switch | -| Scope | personal org, cloud org, removed org, revoked share | -| Session | unopened, active, inactive, deleted, forked | -| Instance | primary, direct-launched secondary, launcher-created secondary | - -Flag any resource whose owner or terminal state is ambiguous. - -### 3. Choose the correct pattern - -Apply the smallest applicable pattern: - -- **Push + safety TTL:** subscribe to authoritative change events; use a slow TTL only to recover missed events. -- **Visibility-aware recursive timeout:** keep at most one timer, clear it while hidden, run once and reschedule on return. Prefer this over overlapping intervals. -- **Single-flight coordinator:** key by identity and resource, share the in-flight promise, carry an invalidation version/generation, and prevent stale completion from overwriting newer state. -- **Bounded LRU/TTL:** refresh recency on read, cap entry count, give failures a short TTL, and provide lifecycle eviction. -- **Per-store state:** use `WeakMap` when multiple Jotai stores or rendered instances can exist in one process. -- **Narrow subscription:** use per-session atoms/selectors or keyed stores rather than reading a global delta map. -- **Burst coalescing:** batch updates once per frame or bounded debounce; preserve terminal/final events. -- **Demand-driven loading:** paginate or fetch details only after expansion/selection; retain only the visible or recently used window. -- **Generation guard:** discard late async results after stop, restart, account switch, endpoint switch, or a newer request. - -### 4. Sweep equivalent paths - -After finding one issue, search for every semantic peer. A fix is incomplete if another surface still owns a parallel implementation. - -Typical ORG2 sweeps: - -- Sidebar + management panel + share dialog + Work Item hooks fetching the same roster -- Visible and hidden polling paths -- Primary launcher and direct secondary executable startup -- Positive, negative, and in-flight cache entries -- Worker success, crash, dispose, session deletion, and app shutdown -- Local session, cloud member session, guest import, fork, and external CLI history -- Production action and rendered E2E action - -Unify duplicate resource ownership before tuning individual call sites. - -### 5. Protect correctness and privacy - -Performance changes must not weaken: - -- realtime propagation after push invalidation -- revocation/removal disappearance -- durable outbox retries and tombstones -- account/endpoint/org data isolation -- first-load and focus-return freshness -- session fork/history integrity -- terminal streaming events - -Capture identity and generation at request start. Before committing a result, confirm the current identity/generation still matches. Do not display a previous identity's cached rows while refreshing. - -### 6. Verify proportionally - -Always run: - -- targeted unit tests for cache bounds, coalescing, invalidation, visibility, and stale-result rejection -- TypeScript typecheck and lint for changed frontend files -- Rust unit tests/checks for changed backend modules; if the shared Cargo cache is corrupt or policy-blocked, report it and use the narrowest valid independent compilation without deleting broad caches -- `git diff --check` - -For rendered/background changes, also run the real Tauri surface when available: - -1. Observe primary and secondary instances separately. -2. Measure visible idle, hidden idle, active streaming, and post-close/post-delete behavior. -3. Exercise account switch, endpoint switch, and direct secondary launch when relevant. -4. Confirm request/subscription/timer counts stabilize rather than grow after repeated open/close cycles. -5. Confirm strict rendered E2E uses user-visible actions for the behavior under assertion. - -Do not claim a performance improvement from code shape alone. State the evidence actually collected and any environment blocker. - -## Review rejection rules - -Reject or revise a change when any applicable answer is unknown or false: - -- Who owns this background resource, and exactly when is it stopped? -- Can this timer overlap itself or continue while hidden? -- Why is polling necessary instead of invalidation? -- Can two mounted consumers issue the same request? -- Does the cache have a maximum size, freshness rule, identity key, and eviction event? -- Can an old async completion write after a newer request or identity switch? -- Does one session's update wake unrelated session views? -- Does a growing transcript/history/diff require full eager materialization? -- Does a direct secondary launch inherit primary external history or auth state? -- Can a missing rendered element be skipped while the E2E still passes? - -## Required delivery output - -Report findings and evidence in this compact form: - -| Area | Verdict | Evidence | Change or reason kept | Verification | -| ------------------ | ---------- | ------------------------------------ | ------------------------- | ----------------------- | -| Background work | fix / keep | timer/subscription owner and cadence | exact lifecycle decision | test or measurement | -| Memory | fix / keep | retained structure and growth bound | cap/TTL/eviction | bound/eviction test | -| Scope/isolation | fix / keep | cache/request key | identity/generation guard | switch/revocation test | -| Rendering/hot path | fix / keep | subscription/allocation trace | narrowing/coalescing | render or unit evidence | - -End with: - -- `Performance verdict: pass` only when every applicable invariant is evidenced. -- `Performance verdict: blocked` when required real measurement or compilation cannot run; name the blocker. -- `Performance verdict: fail` when an unbounded, duplicate, hidden-active, stale-write, or cross-identity path remains. - -Never promise that a skill can make regressions impossible. Enforce the gates, expose unknowns, and refuse an unsupported green verdict. +Do not claim a performance improvement from typecheck or unit tests alone. Provide concrete lifecycle evidence proportional to the changed runtime path. diff --git a/.orgii/skills/org2-performance-guard/references/runtime-patterns.md b/.orgii/skills/org2-performance-guard/references/runtime-patterns.md new file mode 100644 index 000000000..0c9a5a863 --- /dev/null +++ b/.orgii/skills/org2-performance-guard/references/runtime-patterns.md @@ -0,0 +1,45 @@ +# Runtime Performance Patterns + +### 3. Choose the correct pattern + +Apply the smallest applicable pattern: + +- **Push + safety TTL:** subscribe to authoritative change events; use a slow TTL only to recover missed events. +- **Visibility-aware recursive timeout:** keep at most one timer, clear it while hidden, run once and reschedule on return. Prefer this over overlapping intervals. +- **Single-flight coordinator:** key by identity and resource, share the in-flight promise, carry an invalidation version/generation, and prevent stale completion from overwriting newer state. +- **Bounded LRU/TTL:** refresh recency on read, cap entry count, give failures a short TTL, and provide lifecycle eviction. +- **Per-store state:** use `WeakMap` when multiple Jotai stores or rendered instances can exist in one process. +- **Narrow subscription:** use per-session atoms/selectors or keyed stores rather than reading a global delta map. +- **Burst coalescing:** batch updates once per frame or bounded debounce; preserve terminal/final events. +- **Demand-driven loading:** paginate or fetch details only after expansion/selection; retain only the visible or recently used window. +- **Generation guard:** discard late async results after stop, restart, account switch, endpoint switch, or a newer request. + +### 4. Sweep equivalent paths + +After finding one issue, search for every semantic peer. A fix is incomplete if another surface still owns a parallel implementation. + +Typical ORG2 sweeps: + +- Sidebar + management panel + share dialog + Work Item hooks fetching the same roster +- Visible and hidden polling paths +- Primary launcher and direct secondary executable startup +- Positive, negative, and in-flight cache entries +- Worker success, crash, dispose, session deletion, and app shutdown +- Local session, cloud member session, guest import, fork, and external CLI history +- Production action and rendered E2E action + +Unify duplicate resource ownership before tuning individual call sites. + +### 5. Protect correctness and privacy + +Performance changes must not weaken: + +- realtime propagation after push invalidation +- revocation/removal disappearance +- durable outbox retries and tombstones +- account/endpoint/org data isolation +- first-load and focus-return freshness +- session fork/history integrity +- terminal streaming events + +Capture identity and generation at request start. Before committing a result, confirm the current identity/generation still matches. Do not display a previous identity's cached rows while refreshing. diff --git a/.orgii/skills/org2-performance-guard/references/surface-and-lifecycle.md b/.orgii/skills/org2-performance-guard/references/surface-and-lifecycle.md new file mode 100644 index 000000000..1dd167a95 --- /dev/null +++ b/.orgii/skills/org2-performance-guard/references/surface-and-lifecycle.md @@ -0,0 +1,38 @@ +# Performance Surface and Lifecycle + +### 1. Establish the performance surface + +Read the changed call chain from its production entry point. Inventory every resource the change can create or retain: + +- `setInterval`, recursive `setTimeout`, `requestAnimationFrame`, debounce, retry, backoff +- DOM/Tauri/network listeners and Realtime channels +- workers, subprocesses, watchers, file scans, git operations, database reads +- module globals, atom maps, per-store maps, promises, abort controllers, buffers +- React subscriptions, selectors, derived arrays, render-time sorting/grouping +- eager list/history/diff/replay loading + +Use targeted searches, adapting paths to the diff: + +```powershell +rg -n "setInterval|setTimeout|requestAnimationFrame|addEventListener|listen\(|subscribe|channel\(" src src-tauri +rg -n "new Map|new Set|WeakMap|cache|inFlight|buffer|queue|history" src src-tauri +rg -n "poll|refresh|retry|scan|watch|stream|delta|dispose|cleanup|abort" src src-tauri +``` + +Do not treat grep hits as findings. Trace ownership, start conditions, steady-state behavior, and cleanup. + +### 2. Build the lifecycle matrix + +For each resource, record the required behavior in these states: + +| Dimension | States to check | +| --- | --- | +| App | start, idle, active, shutdown | +| Document | visible, hidden, focus return | +| Network | online, offline, retry/backoff | +| Identity | signed out, signed in, refresh, account switch, endpoint switch | +| Scope | personal org, cloud org, removed org, revoked share | +| Session | unopened, active, inactive, deleted, forked | +| Instance | primary, direct-launched secondary, launcher-created secondary | + +Flag any resource whose owner or terminal state is ambiguous. diff --git a/.orgii/skills/org2-performance-guard/references/verification-and-delivery.md b/.orgii/skills/org2-performance-guard/references/verification-and-delivery.md new file mode 100644 index 000000000..50bf5c9e4 --- /dev/null +++ b/.orgii/skills/org2-performance-guard/references/verification-and-delivery.md @@ -0,0 +1,54 @@ +# Performance Verification and Delivery + +### 6. Verify proportionally + +Always run: + +- targeted unit tests for cache bounds, coalescing, invalidation, visibility, and stale-result rejection +- TypeScript typecheck and lint for changed frontend files +- Rust unit tests/checks for changed backend modules; if the shared Cargo cache is corrupt or policy-blocked, report it and use the narrowest valid independent compilation without deleting broad caches +- `git diff --check` + +For rendered/background changes, also run the real Tauri surface when available: + +1. Observe primary and secondary instances separately. +2. Measure visible idle, hidden idle, active streaming, and post-close/post-delete behavior. +3. Exercise account switch, endpoint switch, and direct secondary launch when relevant. +4. Confirm request/subscription/timer counts stabilize rather than grow after repeated open/close cycles. +5. Confirm strict rendered E2E uses user-visible actions for the behavior under assertion. + +Do not claim a performance improvement from code shape alone. State the evidence actually collected and any environment blocker. + +## Review rejection rules + +Reject or revise a change when any applicable answer is unknown or false: + +- Who owns this background resource, and exactly when is it stopped? +- Can this timer overlap itself or continue while hidden? +- Why is polling necessary instead of invalidation? +- Can two mounted consumers issue the same request? +- Does the cache have a maximum size, freshness rule, identity key, and eviction event? +- Can an old async completion write after a newer request or identity switch? +- Does one session's update wake unrelated session views? +- Does a growing transcript/history/diff require full eager materialization? +- Does a direct secondary launch inherit primary external history or auth state? +- Can a missing rendered element be skipped while the E2E still passes? + +## Required delivery output + +Report findings and evidence in this compact form: + +| Area | Verdict | Evidence | Change or reason kept | Verification | +| --- | --- | --- | --- | --- | +| Background work | fix / keep | timer/subscription owner and cadence | exact lifecycle decision | test or measurement | +| Memory | fix / keep | retained structure and growth bound | cap/TTL/eviction | bound/eviction test | +| Scope/isolation | fix / keep | cache/request key | identity/generation guard | switch/revocation test | +| Rendering/hot path | fix / keep | subscription/allocation trace | narrowing/coalescing | render or unit evidence | + +End with: + +- `Performance verdict: pass` only when every applicable invariant is evidenced. +- `Performance verdict: blocked` when required real measurement or compilation cannot run; name the blocker. +- `Performance verdict: fail` when an unbounded, duplicate, hidden-active, stale-write, or cross-identity path remains. + +Never promise that a skill can make regressions impossible. Enforce the gates, expose unknowns, and refuse an unsupported green verdict. diff --git a/.orgii/skills/react-best-practices/SKILL.md b/.orgii/skills/react-best-practices/SKILL.md index e303c1fbd..8610c5d68 100644 --- a/.orgii/skills/react-best-practices/SKILL.md +++ b/.orgii/skills/react-best-practices/SKILL.md @@ -1,215 +1,38 @@ --- name: react-best-practices -description: ORGII-specific React 19 performance review and implementation guidance, adapted from Vercel Engineering. Use for React performance, re-render, async waterfall, bundle, heavy dependency, virtualization, high-frequency event, Context/provider, or store-subscription work. Do not trigger for styling, copy changes, or routine single-file UI bug fixes without a performance concern. -license: MIT (upstream guidance); ORGII overlay follows the repository license -metadata: - upstream: vercel-labs/agent-skills/skills/react-best-practices - upstream-version: "1.0.0" - upstream-revision: dc8367e6f91c022d83361f03c3313fa05e848ee5 - adapted-for: ORGII React 19 + Webpack + Tauri client +description: ORGII-specific React 19 performance review and implementation guidance adapted from Vercel Engineering. Use for React re-render, async waterfall, bundle/startup cost, heavy dependency, virtualization, high-frequency event, Context/provider, persistence, or store-subscription work. Do not trigger for styling, copy changes, or routine single-file UI bug fixes without a performance concern. --- # ORGII React Best Practices -Use Vercel's React performance guidance through this ORGII overlay. ORGII is a React 19 Webpack SPA running inside Tauri, not a Next.js, RSC, or SSR application. This file is authoritative whenever upstream examples conflict with ORGII's runtime, dependencies, architecture, or verification requirements. +Apply React performance guidance for ORGII's React 19, Webpack, and Tauri client runtime. Next.js, React Server Components, SSR, and server-only advice do not apply. -See `UPSTREAM.md` for provenance and the pinned upstream source. +See [UPSTREAM.md](UPSTREAM.md) only when provenance or comparison with the pinned upstream guidance is required. -## When To Use +## Applicability filter -Load this skill when the task involves one or more of: +- Require a concrete performance concern: render frequency, async sequencing, startup/bundle cost, high-frequency browser work, subscription scope, virtualization, or persistence overhead. +- Prefer measured evidence over generic optimization claims. +- Preserve ORGII's existing state ownership and data-access architecture. +- Do not introduce SWR, server-only APIs, or a new state architecture solely to follow upstream examples. +- Do not trade correctness, accessibility, or lifecycle cleanup for a smaller benchmark number. -- React performance, lag, responsiveness, unnecessary renders, or profiling -- Async waterfalls or independent work that may safely run in parallel -- Bundle size, startup cost, lazy loading, or a new heavy dependency -- Large lists, search/filter/sort, virtualization, CodeMirror, or xterm -- Context/provider value stability, Jotai subscriptions, or derived state -- Global listeners, timers, animation frames, scroll, pointer, resize, or other high-frequency events -- A React performance-focused review or refactor +## Priority -## When Not To Use +1. Eliminate serial async work and repeated I/O. +2. Narrow subscriptions and prevent broad re-render fan-out. +3. Remove heavy eager imports from startup paths. +4. Bound high-frequency work and large rendered collections. +5. Apply micro-optimizations only after the larger costs are measured. -Do not load this skill solely for: +## Workflow -- Styling, copy, spacing, colors, or design-system consistency -- Accessibility review without a performance concern -- A routine single-file UI bug fix -- Backend-only Rust work -- General architecture cleanup without a React performance dimension +1. Establish a baseline and identify the hot path or expensive lifecycle. +2. Read [implementation-guidance.md](references/implementation-guidance.md) for applicable async, render, browser, import, data-fetching, subscription, and persistence patterns. +3. Read [compatibility-boundaries.md](references/compatibility-boundaries.md) before applying upstream advice that assumes Next.js, RSC, SSR, server caching, or framework-specific loaders. +4. Read [high-risk-surfaces.md](references/high-risk-surfaces.md) when touching the named ORGII subsystems. +5. Follow [workflow-and-verification.md](references/workflow-and-verification.md) for measurement, implementation order, regression coverage, and delivery reporting. -Use `frontend-ui-audit` for UI consistency and accessibility methodology. Use `architecture-audit` for broader state ownership, module boundaries, dead code, FSM, or cross-layer refactors. This skill does not replace either one. +## Delivery -## ORGII Applicability Filter - -Before applying any rule, classify it: - -| Classification | Action | -| --------------------- | ---------------------------------------------------------------------------------------------------------------------- | -| **Applicable** | Fits React 19 in a client-side Webpack/Tauri runtime; evaluate normally. | -| **Adapt** | The principle fits but the upstream implementation is Next.js/SWR-specific; translate it to existing ORGII primitives. | -| **Evidence required** | A micro-optimization, lifecycle-sensitive change, or high-risk surface; measure or reproduce before changing. | -| **Not applicable** | Depends on Next.js, RSC, SSR, Server Actions, API routes, or server request lifecycle; do not apply. | - -Never treat a rule match as automatic authorization to refactor. Preserve behavior first and prefer the smallest measurable change. - -## Default Priority for ORGII - -Review in this order: - -1. **Correctness and lifecycle stability** — stale closures, duplicate effects/listeners, remounts, races, and cleanup. -2. **Async waterfalls** — defer unused awaits and parallelize only genuinely independent operations. -3. **Subscription and render scope** — subscribe to the smallest stable value; derive values during render when safe. -4. **Heavy work and bundle boundaries** — delay expensive modules and computations until the feature is used. -5. **Large or frequent surfaces** — virtualized rows, editor/terminal events, search/filter loops, resize/scroll/pointer paths. -6. **Micro-optimizations** — only after evidence shows the preceding categories are not the bottleneck. - -## Applicable Upstream Guidance - -### Async work - -- Check cheap synchronous exit conditions before awaiting remote or expensive work. -- Move `await` into the branch that needs its result. -- Use `Promise.all` for independent operations; preserve ordering and failure semantics. -- Start independent work early and await it at the latest safe point. - -Do not use `better-all` unless it is deliberately approved as a new dependency. Native promises are the default. - -### Re-render and state - -- Derive values from current props/state during render instead of mirroring them through an effect. -- Put interaction-triggered side effects in the event handler rather than modeling the action as state plus effect. -- Use functional state updates when the next value depends on the previous value. -- Lazily initialize expensive state with `useState(() => initialValue)`. -- Narrow effect dependencies to the actual primitive values, without suppressing legitimate dependencies. -- Do not define component types inside another component when remounting is not intentional. -- Split unrelated computations/effects when their dependency lifecycles differ. -- Do not wrap cheap primitive expressions in `useMemo`. -- Use `startTransition` or `useDeferredValue` only for demonstrably non-urgent rendering work; do not hide correctness or stale-data issues. -- Stabilize Context provider values when unstable identity fans out renders to consumers. - -`memo`, `useMemo`, and `useCallback` are tools, not defaults. Add them only when they create a meaningful bailout or stable contract. Confirm whether React Compiler is enabled before relying on compiler-provided memoization; do not assume it is enabled. - -### Rendering and high-frequency browser work - -- Prefer existing virtualization (`react-virtuoso`, `@tanstack/react-virtual`) for large lists rather than rendering every item. -- Consider `content-visibility` only where it is compatible with measurement, focus, scrolling, and virtualization behavior. -- Hoist truly static JSX or stable default arrays/objects/functions when identity matters. -- Use passive touch/wheel listeners only when the listener never calls `preventDefault()`. -- Batch DOM style mutations through classes where imperative DOM work is required. -- Clean up listeners, observers, timers, animation frames, terminal/editor subscriptions, and async continuations symmetrically. - -### JavaScript hot paths - -Use `Map`/`Set`, combined iterations, cached lookups, hoisted regular expressions, or hand-written loops only when data volume or profiling justifies them. Prefer readable immutable code on ordinary UI paths. - -## Required Adaptations - -### Dynamic imports - -Do not use `next/dynamic`. Use Webpack-compatible imports: - -```tsx -import { Suspense, lazy } from "react"; - -const HeavyPanel = lazy(() => import("./HeavyPanel")); - -export function PanelHost() { - return ( - }> - - - ); -} -``` - -For event-triggered utilities, use `await import("./heavyUtility")` at the point of use. Preserve error handling and avoid turning a frequently used path into repeated load churn. - -### Data fetching and subscriptions - -Do not introduce SWR, React Query, or another cache/subscription framework just to satisfy an upstream example. First reuse the existing owner: - -- Jotai atoms and derived atoms -- Existing Context/provider contracts -- Existing module service/cache/store -- Tauri command and event ownership -- Existing deduplication or in-flight request logic - -A new data library requires an explicit architecture decision and migration boundary. - -### Bundle imports - -Direct imports are candidates, not a blanket ban on barrel files. Before rewriting imports: - -1. Inspect whether Webpack tree-shakes the package/module correctly. -2. Measure the affected chunk with the existing `pnpm analyze` path when bundle impact is the claim. -3. Preserve public module boundaries where the barrel is an intentional API. -4. Prefer lazy feature boundaries over noisy import churn with no measured result. - -### Persistence - -For browser storage, prefer ORGII's existing persistence owner. If direct `localStorage` or `sessionStorage` access is necessary, version the schema, store minimal non-sensitive data, and handle read/write failures. Never persist credentials, OAuth tokens, or KeyVault secrets there. - -## Explicitly Inapplicable Upstream Rules - -Do not apply the following to ORGII's frontend unless the architecture later adds the relevant runtime: - -- Next.js API route or Server Action patterns -- React Server Components and RSC prop serialization -- `next/dynamic`, `next/server`, `after()`, `next/headers`, or `next/cache` -- Per-request server deduplication with `React.cache()` -- Cross-request LRU server caches -- SSR hydration mismatch or no-flicker inline-script patterns -- Server component composition for parallel server fetching -- Next.js resource, route, image, font, or script behavior - -React `Suspense` remains usable for client-side lazy boundaries, but upstream streaming/RSC claims do not transfer to Tauri. - -## High-Risk ORGII Surfaces - -Do not make speculative performance refactors in these areas. Read owners/callers, preserve lifecycle semantics, and add focused verification: - -- ChatPanel send, queue, Stop, Force Send, rewind, and turn lifecycle -- Composer, ComposerBar, contenteditable input, slash/context menus, and draft restoration -- CodeMirror editor state, extensions, listeners, measurements, and document synchronization -- xterm creation/disposal, addons, WebGL fallback, fit/resize, and stream subscriptions -- Virtuoso or TanStack Virtual row identity, measurement, scroll restoration, and follow-output behavior -- Tooltip/Menu/Dropdown portals, focus, positioning, and outside-click listeners -- WorkStation shell, replay, diff, and multi-repo state ownership -- Tauri IPC and event subscriptions -- KeyVault forms, validation, secrets, and parent-owned loading/error state - -For these surfaces, a lower render count is not sufficient proof. Verify the user-visible lifecycle and authoritative state. - -## Working Method - -1. **State the performance claim.** Name the affected interaction and expected improvement. -2. **Find the owner.** Trace the state, event, async, or bundle boundary before editing. -3. **Establish evidence.** Use a reproducible symptom, render observation, bundle analyzer, browser performance trace, or focused benchmark when practical. -4. **Classify each candidate.** Applicable, adapt, evidence required, or not applicable. -5. **Choose the smallest safe change.** Do not combine unrelated optimization classes. -6. **Sweep equivalent callers.** Classify remaining hits as fix, keep with reason, or not applicable; do not silently stop at the reported site. -7. **Verify correctness first.** Run focused tests, changed-file lint/type diagnostics, and the relevant rendered path when the claim is visual or interaction-based. -8. **Re-measure the original claim.** Do not report a performance improvement solely because code now resembles a best-practice example. - -## Verification and Reporting - -Match verification to the claim: - -| Claim | Minimum evidence | -| ------------------------------------ | ---------------------------------------------------------------------------------------------------- | -| Removed render/remount issue | Focused regression test where feasible plus before/after render or lifecycle observation | -| Removed async waterfall | Focused test for ordering/failure semantics plus timing or call-order evidence | -| Reduced bundle/startup cost | `pnpm analyze` or equivalent chunk evidence before and after | -| Improved long-list interaction | Reproduce realistic data volume and verify scrolling, focus, selection, and empty/single-item states | -| Fixed listener/subscription overhead | Prove one registration per intended owner and symmetric cleanup | -| Improved live UI responsiveness | Run the actual Tauri/WebView path or explicitly state that live pixels/profiling were not verified | - -Do not claim runtime, WebView, startup, memory, or frame-time improvements from TypeScript, lint, or unit tests alone. If measurement was not possible, report the change as an implementation candidate with correctness checks, not a verified performance win. - -## Relationship to ORGII Delivery Rules - -- This skill is performance methodology, not an audit-report mandate. -- It does not replace `.cursor/rules/ui-feature-workflow.mdc` test and acceptance gates. -- It does not require a report for every React edit. -- If a task is explicitly audit-only, keep source changes separate from the audit document. -- When performance and UI consistency both matter, apply both methodologies but keep findings clearly categorized. +State the measured concern, selected rule, ORGII-specific adaptation, verification evidence, and remaining performance risk. Typecheck-only evidence does not prove a runtime performance improvement. diff --git a/.orgii/skills/react-best-practices/references/compatibility-boundaries.md b/.orgii/skills/react-best-practices/references/compatibility-boundaries.md new file mode 100644 index 000000000..809fdc07d --- /dev/null +++ b/.orgii/skills/react-best-practices/references/compatibility-boundaries.md @@ -0,0 +1,16 @@ +# ORGII React Compatibility Boundaries + +## Explicitly Inapplicable Upstream Rules + +Do not apply the following to ORGII's frontend unless the architecture later adds the relevant runtime: + +- Next.js API route or Server Action patterns +- React Server Components and RSC prop serialization +- `next/dynamic`, `next/server`, `after()`, `next/headers`, or `next/cache` +- Per-request server deduplication with `React.cache()` +- Cross-request LRU server caches +- SSR hydration mismatch or no-flicker inline-script patterns +- Server component composition for parallel server fetching +- Next.js resource, route, image, font, or script behavior + +React `Suspense` remains usable for client-side lazy boundaries, but upstream streaming/RSC claims do not transfer to Tauri. diff --git a/.orgii/skills/react-best-practices/references/high-risk-surfaces.md b/.orgii/skills/react-best-practices/references/high-risk-surfaces.md new file mode 100644 index 000000000..842186e70 --- /dev/null +++ b/.orgii/skills/react-best-practices/references/high-risk-surfaces.md @@ -0,0 +1,17 @@ +# High-Risk ORGII React Surfaces + +## High-Risk ORGII Surfaces + +Do not make speculative performance refactors in these areas. Read owners/callers, preserve lifecycle semantics, and add focused verification: + +- ChatPanel send, queue, Stop, Force Send, rewind, and turn lifecycle +- Composer, ComposerBar, contenteditable input, slash/context menus, and draft restoration +- CodeMirror editor state, extensions, listeners, measurements, and document synchronization +- xterm creation/disposal, addons, WebGL fallback, fit/resize, and stream subscriptions +- Virtuoso or TanStack Virtual row identity, measurement, scroll restoration, and follow-output behavior +- Tooltip/Menu/Dropdown portals, focus, positioning, and outside-click listeners +- WorkStation shell, replay, diff, and multi-repo state ownership +- Tauri IPC and event subscriptions +- KeyVault forms, validation, secrets, and parent-owned loading/error state + +For these surfaces, a lower render count is not sufficient proof. Verify the user-visible lifecycle and authoritative state. diff --git a/.orgii/skills/react-best-practices/references/implementation-guidance.md b/.orgii/skills/react-best-practices/references/implementation-guidance.md new file mode 100644 index 000000000..3f5a06574 --- /dev/null +++ b/.orgii/skills/react-best-practices/references/implementation-guidance.md @@ -0,0 +1,87 @@ +# ORGII React Performance Implementation Guidance + +## Applicable Upstream Guidance + +### Async work + +- Check cheap synchronous exit conditions before awaiting remote or expensive work. +- Move `await` into the branch that needs its result. +- Use `Promise.all` for independent operations; preserve ordering and failure semantics. +- Start independent work early and await it at the latest safe point. + +Do not use `better-all` unless it is deliberately approved as a new dependency. Native promises are the default. + +### Re-render and state + +- Derive values from current props/state during render instead of mirroring them through an effect. +- Put interaction-triggered side effects in the event handler rather than modeling the action as state plus effect. +- Use functional state updates when the next value depends on the previous value. +- Lazily initialize expensive state with `useState(() => initialValue)`. +- Narrow effect dependencies to the actual primitive values, without suppressing legitimate dependencies. +- Do not define component types inside another component when remounting is not intentional. +- Split unrelated computations/effects when their dependency lifecycles differ. +- Do not wrap cheap primitive expressions in `useMemo`. +- Use `startTransition` or `useDeferredValue` only for demonstrably non-urgent rendering work; do not hide correctness or stale-data issues. +- Stabilize Context provider values when unstable identity fans out renders to consumers. + +`memo`, `useMemo`, and `useCallback` are tools, not defaults. Add them only when they create a meaningful bailout or stable contract. Confirm whether React Compiler is enabled before relying on compiler-provided memoization; do not assume it is enabled. + +### Rendering and high-frequency browser work + +- Prefer existing virtualization (`react-virtuoso`, `@tanstack/react-virtual`) for large lists rather than rendering every item. +- Consider `content-visibility` only where it is compatible with measurement, focus, scrolling, and virtualization behavior. +- Hoist truly static JSX or stable default arrays/objects/functions when identity matters. +- Use passive touch/wheel listeners only when the listener never calls `preventDefault()`. +- Batch DOM style mutations through classes where imperative DOM work is required. +- Clean up listeners, observers, timers, animation frames, terminal/editor subscriptions, and async continuations symmetrically. + +### JavaScript hot paths + +Use `Map`/`Set`, combined iterations, cached lookups, hoisted regular expressions, or hand-written loops only when data volume or profiling justifies them. Prefer readable immutable code on ordinary UI paths. + +## Required Adaptations + +### Dynamic imports + +Do not use `next/dynamic`. Use Webpack-compatible imports: + +```tsx +import { Suspense, lazy } from "react"; + +const HeavyPanel = lazy(() => import("./HeavyPanel")); + +export function PanelHost() { + return ( + }> + + + ); +} +``` + +For event-triggered utilities, use `await import("./heavyUtility")` at the point of use. Preserve error handling and avoid turning a frequently used path into repeated load churn. + +### Data fetching and subscriptions + +Do not introduce SWR, React Query, or another cache/subscription framework just to satisfy an upstream example. First reuse the existing owner: + +- Jotai atoms and derived atoms +- Existing Context/provider contracts +- Existing module service/cache/store +- Tauri command and event ownership +- Existing deduplication or in-flight request logic + +A new data library requires an explicit architecture decision and migration boundary. + +### Bundle imports + +Direct imports are candidates, not a blanket ban on barrel files. Before rewriting imports: + +1. Inspect whether Webpack tree-shakes the package/module correctly. +2. Measure the affected chunk with the existing `pnpm analyze` path when bundle impact is the claim. +3. Preserve public module boundaries where the barrel is an intentional API. +4. Prefer lazy feature boundaries over noisy import churn with no measured result. + +### Persistence + +For browser storage, prefer ORGII's existing persistence owner. If direct `localStorage` or `sessionStorage` access is necessary, version the schema, store minimal non-sensitive data, and handle read/write failures. Never persist credentials, OAuth tokens, or KeyVault secrets there. diff --git a/.orgii/skills/react-best-practices/references/workflow-and-verification.md b/.orgii/skills/react-best-practices/references/workflow-and-verification.md new file mode 100644 index 000000000..25c959c7b --- /dev/null +++ b/.orgii/skills/react-best-practices/references/workflow-and-verification.md @@ -0,0 +1,35 @@ +# React Performance Workflow and Verification + +## Working Method + +1. **State the performance claim.** Name the affected interaction and expected improvement. +2. **Find the owner.** Trace the state, event, async, or bundle boundary before editing. +3. **Establish evidence.** Use a reproducible symptom, render observation, bundle analyzer, browser performance trace, or focused benchmark when practical. +4. **Classify each candidate.** Applicable, adapt, evidence required, or not applicable. +5. **Choose the smallest safe change.** Do not combine unrelated optimization classes. +6. **Sweep equivalent callers.** Classify remaining hits as fix, keep with reason, or not applicable; do not silently stop at the reported site. +7. **Verify correctness first.** Run focused tests, changed-file lint/type diagnostics, and the relevant rendered path when the claim is visual or interaction-based. +8. **Re-measure the original claim.** Do not report a performance improvement solely because code now resembles a best-practice example. + +## Verification and Reporting + +Match verification to the claim: + +| Claim | Minimum evidence | +| ------------------------------------ | ---------------------------------------------------------------------------------------------------- | +| Removed render/remount issue | Focused regression test where feasible plus before/after render or lifecycle observation | +| Removed async waterfall | Focused test for ordering/failure semantics plus timing or call-order evidence | +| Reduced bundle/startup cost | `pnpm analyze` or equivalent chunk evidence before and after | +| Improved long-list interaction | Reproduce realistic data volume and verify scrolling, focus, selection, and empty/single-item states | +| Fixed listener/subscription overhead | Prove one registration per intended owner and symmetric cleanup | +| Improved live UI responsiveness | Run the actual Tauri/WebView path or explicitly state that live pixels/profiling were not verified | + +Do not claim runtime, WebView, startup, memory, or frame-time improvements from TypeScript, lint, or unit tests alone. If measurement was not possible, report the change as an implementation candidate with correctness checks, not a verified performance win. + +## Relationship to ORGII Delivery Rules + +- This skill is performance methodology, not an audit-report mandate. +- It does not replace `.cursor/rules/ui-feature-workflow.mdc` test and acceptance gates. +- It does not require a report for every React edit. +- If a task is explicitly audit-only, keep source changes separate from the audit document. +- When performance and UI consistency both matter, apply both methodologies but keep findings clearly categorized.