From 61509f1b95c29a134a86faab11cbbf8c72a3b8f2 Mon Sep 17 00:00:00 2001 From: Loretta Date: Fri, 24 Apr 2026 08:21:49 +0200 Subject: [PATCH] [LOADED_DOCS: NONE] Update protocol, docs-discovery skill, and doc layering - Switched AI AGENT CORE PROTOCOL to new `[LOADED_DOCS: N files (+K this turn: ...)]` prefix format in all primary instruction files; updated examples and enforcement details. - Added `docs-discovery` skill for proactive .md doc loading before code search; documented usage and integration as a shared agent skill. - Introduced `## Protocol History` section and `LLM_PROTOCOL_DECISIONS.md` decision log for cross-repo protocol change tracking. - Expanded protocol-audit skill and `REPOS.md` to support 8 instruction files (primary/inherit split), new invariants, and known issues. - Added/updated structured TODO and issues docs for serialization, logging, and SignalR binary protocol. - Improved cross-references, doc layering, and clarified documentation-first coding workflow. - Various minor doc clarifications and formatting for protocol consistency. --- .github/LLM_PROTOCOL_DECISIONS.md | 79 +++++++++ .github/copilot-instructions.md | 21 ++- .github/skills/docs-discovery/SKILL.md | 103 ++++++++++++ .github/skills/protocol-audit/SKILL.md | 156 ++++++++++++------ .../skills/protocol-audit/references/REPOS.md | 76 +++++++-- AyCode.Core/docs/BINARY_ISSUES.md | 48 ++++++ AyCode.Core/docs/BINARY_TODO.md | 18 ++ AyCode.Core/docs/BINARY_WRITERS.md | 5 +- AyCode.Core/docs/LOGGING.md | 83 ++++++++++ AyCode.Core/docs/LOGGING_ISSUES.md | 155 +++++++++++++++++ AyCode.Core/docs/LOGGING_TODO.md | 85 ++++++++++ AyCode.Services/docs/SIGNALR.md | 4 +- .../docs/SIGNALR_BINARY_PROTOCOL_ISSUES.md | 34 ++++ .../docs/SIGNALR_BINARY_PROTOCOL_TODO.md | 46 ++++++ AyCode.Services/docs/SIGNALR_ISSUES.md | 41 +++++ AyCode.Services/docs/SIGNALR_TODO.md | 31 ++++ docs/CONVENTIONS.md | 7 +- 17 files changed, 908 insertions(+), 84 deletions(-) create mode 100644 .github/LLM_PROTOCOL_DECISIONS.md create mode 100644 .github/skills/docs-discovery/SKILL.md create mode 100644 AyCode.Core/docs/BINARY_TODO.md create mode 100644 AyCode.Core/docs/LOGGING_ISSUES.md create mode 100644 AyCode.Core/docs/LOGGING_TODO.md create mode 100644 AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_ISSUES.md create mode 100644 AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_TODO.md create mode 100644 AyCode.Services/docs/SIGNALR_TODO.md diff --git a/.github/LLM_PROTOCOL_DECISIONS.md b/.github/LLM_PROTOCOL_DECISIONS.md new file mode 100644 index 0000000..1cdc9cc --- /dev/null +++ b/.github/LLM_PROTOCOL_DECISIONS.md @@ -0,0 +1,79 @@ +# LLM Protocol Decisions + +Append-only log of decisions about the AI AGENT CORE PROTOCOL and related LLM tooling (skills, instruction files, shared conventions) across the 8 known `.github/copilot-instructions.md` files. + +## Purpose + +Quick lookup for **why** a rule is the way it is, to avoid: +- Re-debating decisions that were already discussed and resolved +- Silently "optimizing away" a rule whose edge-case purpose is no longer obvious +- Losing context between sessions / across team members + +## Scope + +**Tracked here:** +- `.github/copilot-instructions.md` rule additions / removals / rewordings +- `CLAUDE.md` changes +- New, modified, or removed agent skills (`docs-discovery`, `protocol-audit`, etc.) +- Protocol-level structural shifts (doc layout, file location conventions) + +**NOT tracked here:** +- Code refactors or feature work — use commit messages / PR descriptions +- Code-architecture decisions — see `docs/ARCHITECTURE.md` at the relevant layer +- Trivial fixes (typos, path corrections without rule-semantic change) + +## Writing rules + +1. **Append-only.** If a decision is reversed or superseded, add a NEW entry that points to the old one. Do not rewrite history — the evolution itself is informative. +2. **One row per decision.** Keep the rationale concise; the purpose is scan-ability, not prose. +3. **`Affected` column** uses the scope codes defined below — always be explicit about which files were touched. +4. **Explicit consent required for additions** (per Rule #5 of the active `copilot-instructions.md`). The LLM proposes the entry; the user reviews and approves before the write. +5. **Reference this log** before proposing a protocol change — if the change was already discussed, the existing entry may save a debate. + +## Scope codes (for the `Affected` column) + +| Code | Meaning | +|------------------------|-----------------------------------------------------------------------------------------------------------| +| `5× primary` | All 5 primary `copilot-instructions.md` (AyCode.Core, AyCode.Blazor, Libraries, FruitBank, FruitBankHybridApp) | +| `4× non-Core primary` | The 4 primary files except AyCode.Core | +| `3× inherit` | The 3 inherit files (Mango.Nop.Core, Nop.Plugin.Misc.AIPlugin, Mango.FruitBank) | +| `all 8` | All 8 files: 5 primary + 3 inherit | +| `/...` | A single specific file path | +| `/...` | Skill directory under `.github/skills/` | + +The "primary" vs "inherit" distinction is defined in `protocol-audit/references/REPOS.md`. + +## 2026 + +| Date | Decision | Rationale | Affected | +|------------|-------------------------------------------------------|---------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------| +| 2026-04-20 | Added CROSS-REPO HARD-GATE to Rule #2 | Docs-before-code was applied only to the own repo, not external deps | `5× primary` | +| 2026-04-20 | Added PER-QUESTION DOC-FIRST to Rule #2 | LLM skipped checking for relevant `.md` before jumping to code search | `5× primary` | +| 2026-04-21 | Expanded Rule #5 scope: "code/files" → "any file..." | Memory / config / docs edits also need consent; old wording too narrow | `5× primary` | +| 2026-04-22 | Added Auto-detection triggers to Rule #4 | "realize" was subjective; post-compaction reset to `[LOADED_DOCS: NONE]` didn't fire reliably | `5× primary` | +| 2026-04-22 | Added "in context" definition to Rule #3 | Summary content was mistaken for actually-loaded docs (lossy compression) | `5× primary` | +| 2026-04-22 | Unified AyCode.Blazor Rule #3 with the other 4 repos | Was custom "CRITICAL TOOL EXECUTION FIREWALL" variant; normalized | `AyCode.Blazor/copilot-instructions.md` | +| 2026-04-22 | "strictly maintain rule X" → "rule 3" | Per-repo numbers varied (15/18/19/20/21); unified reference | `5× primary` | +| 2026-04-22 | Created `protocol-audit` skill at repo-level | Cross-repo consistency check; `.github/skills/` chosen over personal paths (DRY) | `protocol-audit/SKILL.md` + `protocol-audit/references/REPOS.md` | +| 2026-04-22 | Added `## Shared Agent Skills` section (protocol-audit) | Discoverability of AyCode.Core-hosted skills from dependent repos | `4× non-Core primary` | +| 2026-04-23 | Created `docs-discovery` skill (parallel session) | Paired-doc (`main` + `_ISSUES` + `_TODO`) auto-loading before code search | `docs-discovery/SKILL.md` + `AyCode.Core/copilot-instructions.md` | +| 2026-04-23 | Extended `## Shared Agent Skills` with `docs-discovery` (parallel session) | Both skills now listed in each dependent repo | `4× non-Core primary` + `Mango.Nop.Core/copilot-instructions.md` | +| 2026-04-24 | LOADED_DOCS prefix: full list → count+delta format | Long lists became visual noise at scale; delta preserves self-commitment | `5× primary` + `docs-discovery/SKILL.md` | +| 2026-04-24 | Created this Decision Log | Institutional memory for protocol evolution; avoid re-debating resolved choices | `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md` | +| 2026-04-24 | Added `## Protocol History` section | Cross-repo discoverability of the Decision Log | `4× non-Core primary` + `Mango.Nop.Core/copilot-instructions.md` | +| 2026-04-24 | Filled empty Nop.Plugin.Misc.AIPlugin instruction file | Previously 0 bytes; now minimal inherit-pattern referencing AyCode.Core + both skills + log | `Nop.Plugin.Misc.AIPlugin/copilot-instructions.md` | +| 2026-04-24 | Filled empty Mango\FruitBank instruction file | Previously 0 bytes; now minimal inherit-pattern with ⚠️ "purpose TBD" notice | `Mango/FruitBank/copilot-instructions.md` | +| 2026-04-24 | Expanded REPOS.md: 5 repos → 8 (primary + inherit) | Audit scope was incomplete; sub-project & plugin files were missed | `protocol-audit/references/REPOS.md` | +| 2026-04-24 | Fixed Mango.Nop.Core `@repo` path (7 `..` → 6 `..`) | Previous path resolved to `H:\Aycode\...` (non-existent). Corrected relative depth from repo root | `Mango.Nop.Core/copilot-instructions.md` | +| 2026-04-24 | Resolved Mango.FruitBank purpose (nopCommerce host) | Confirmed: directory is a nopCommerce deployment for FruitBank company, hosting Nop.Plugin.Misc.AIPlugin. Layer 4 (host), not TBD. Updated content accordingly. | `Mango/FruitBank/copilot-instructions.md` | +| 2026-04-24 | `protocol-audit` v1.0 → v2.0: primary/inherit invariant split | SKILL.md now applies Common + Primary invariants to rows 1-5, Common + Inherit + Cross-cutting to rows 6-8. New invariants: P3 (Rule #1 count+delta format), X1 (Shared Agent Skills), X2 (Protocol History). | `protocol-audit/SKILL.md` + `protocol-audit/references/REPOS.md` (issues cleared) | + +## Known follow-ups + +*(No open follow-ups. All items from previous audits resolved — see dated entries above for fix history.)* + +## Notes + +- Dates before 2026-04-24 are retroactive reconstructions from session history; intra-day ordering is approximate. +- For precise attribution of individual file changes, consult the git history of the affected files. +- This log is a **summary** artifact — it records the decision and its rationale, not the full diff. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0d47173..47b3659 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -3,9 +3,20 @@ ## 🛑 AI AGENT CORE PROTOCOL (CRITICAL ENFORCEMENT) You are operating in a multi-repo, documentation-first architecture. You MUST STRICTLY follow this protocol for every response. Failure to do so will break the workspace rules. -1. **MANDATORY OUTPUT PREFIX:** Your response MUST begin exactly with this format on the very first line: - `[LOADED_DOCS: ]` - *(If no .md files are loaded yet, write `[LOADED_DOCS: NONE]`.)* +1. **MANDATORY OUTPUT PREFIX:** Your response MUST begin on the very first line with this format: + `[LOADED_DOCS: N files (+K this turn: )]` + - `N` = total count of `.md` files currently in your context (across all loaded docs in this conversation) + - `K` = count of `.md` files newly loaded during THIS response (may be 0) + - If `K > 0`: list the newly loaded **basenames only** (no paths) after `:` + - If `K = 0`: write `[LOADED_DOCS: N files, no new loads]` + - If `N = 0`: write `[LOADED_DOCS: NONE]` + + **Examples:** + - `[LOADED_DOCS: NONE]` — nothing loaded yet + - `[LOADED_DOCS: 1 files, no new loads]` — only `copilot-instructions.md` loaded earlier, nothing new this turn + - `[LOADED_DOCS: 4 files (+3 this turn: LOGGING.md, LOGGING_ISSUES.md, LOGGING_TODO.md)]` — 3 new this turn + + This prefix is MANDATORY on **EVERY** response (not just the first, not just when loading happens). It serves two purposes: **(a)** user-visible compliance signal, and **(b)** self-commitment for the no-re-read rule — in subsequent turns you read your own prior prefix from the conversation to enforce Rule #3. Dropping the prefix breaks both. 2. **HARD-GATE DELAY (DOCS BEFORE CODE) & TOOL EXECUTION BLOCK:** - If `[LOADED_DOCS: NONE]` applies, you **MUST STOP** and you are **STRICTLY FORBIDDEN** to use the following tools: `code_search`, `get_symbols_by_name`, `find_symbol`, or `get_file` (for non-markdown files). @@ -39,6 +50,10 @@ You are operating in a multi-repo, documentation-first architecture. You MUST ST 5. **EXPLICIT CONSENT FOR MODIFICATIONS:** NEVER rewrite, create, or delete any file (code, documentation, configuration, memory, or otherwise) without the user's explicit permission. If the user does not specifically request a code modification (e.g., using phrases like "we are just thinking," "what do you think," "let's plan"), you MUST ONLY provide text-based analysis and planning. You are FORBIDDEN from using file-modifying tools (`replace_string_in_file`, `edit_file`, `create_file`, etc.) until the user explicitly says "ok", "go ahead", "implement", or a similar unambiguous approval. +## Documentation-first coding + +Before running any source-code `Grep` / `get_file` / `code_search` in response to a domain-related user request, invoke the **`docs-discovery`** skill (`.github/skills/docs-discovery/SKILL.md`). It scans `docs/` folders via Glob using topic tokens extracted from the request, loads paired main/`_ISSUES`/`_TODO` .md sets as a unit, and honours the **no-re-read** rule. This saves tokens and prevents reintroducing fixed bugs. The skill contains the full procedure; this rule is just the trigger. + ## Workspace Dependencies @repo { diff --git a/.github/skills/docs-discovery/SKILL.md b/.github/skills/docs-discovery/SKILL.md new file mode 100644 index 0000000..a41f261 --- /dev/null +++ b/.github/skills/docs-discovery/SKILL.md @@ -0,0 +1,103 @@ +--- +name: docs-discovery +description: Load all .md documentation relevant to the user's current coding task BEFORE searching source code or making modifications. Scans `docs/` folders via Glob using topic keywords extracted from the user's message, loads paired main/ISSUES/TODO .md sets as one unit, and respects the no-re-read rule (skips already-loaded files). Use when the user's request mentions any domain concept, class name, file area, or feature behavior — invoke BEFORE the first `code_search`/`get_file`/`Grep` on source files. Typical triggers: any coding/refactoring task, "let's fix X", "review Y", "how does Z work", or any question that would otherwise lead to source-code exploration. +compatibility: Designed for Claude Code and GitHub Copilot (VS). Uses the host agent's Glob/Read tools. +metadata: + author: Fullepi + version: "1.0" +--- + +# docs-discovery + +Ensure relevant `.md` documentation is loaded **before** any source-code search or modification, so the LLM has the documented behaviour, known issues, and planned work in context. Saves many `Grep` / `get_file` rounds — reading a handful of .md files upfront is cheaper than rediscovering information via code search. + +## Before you start + +This skill READS `.md` files and updates the LLM's `[LOADED_DOCS: ...]` state. It MUST NOT modify any file. Follow the **no-re-read** and **explicit-consent-for-modifications** rules from the active repo's `copilot-instructions.md` (rule numbers may differ per repo — refer to the rule NAMES). + +## Step 1 — Extract topic keywords + +Parse the user's most recent message (and the wider conversation tail if relevant) for concrete concepts. Examples: + +- Class / type names: `AcLoggerBase`, `SegmentBufferReader`, `AcBinaryHubProtocol`, `FruitBankSignalRClient` +- Feature areas: "logger", "log writer", "serializer", "SignalR", "hub protocol", "chunked framing", "connection builder", "options" +- File hints: `Program.cs`, `AcLoggerBase.cs`, `SIGNALR.md` +- Patterns / idioms: "DI factory", "appsettings", "mode negotiation" + +Derive **root topic tokens** from these — singular, lowercase, domain-defining words: +- `"AcLoggerBase"` → `logger`, `logging` +- `"SignalR client"` → `signalr` +- `"AcBinarySerializer"` → `binary`, `serializer` +- `"AcBinaryHubProtocol"` → `protocol`, `signalr_binary_protocol`, `binary` +- `"chunked"` → `signalr_binary_protocol` + +Keep the set small (usually 1-3 root tokens). If the request genuinely spans multiple domains, include all. + +## Step 2 — Map tokens to glob patterns (semantic, not hardcoded) + +For each root token, synthesize `.md` filename patterns using common conventions: + +| Token example | Glob patterns to try | +|---|---| +| `logger`, `log`, `logging` | `**/docs/LOGGING*.md` | +| `binary`, `serializer` | `**/docs/BINARY*.md` | +| `signalr`, `hub` | `**/docs/SIGNALR*.md` | +| `protocol`, `wire`, `chunked` | `**/docs/*PROTOCOL*.md` | +| `grid`, `mggrid` | `**/docs/MGGRID*.md` | + +Do NOT require the tokens to match a pre-baked list — construct patterns from the token itself uppercased (e.g., `logger` → `**/docs/LOGGER*.md` + `**/docs/LOGGING*.md`). Natural language variants (logger/logging, serialize/serializer, binary/binaries) should all be attempted. + +Also consider suffix patterns: +- `**/docs/*{TOKEN}*.md` (substring match) +- `**/docs/*{TOKEN}_ISSUES.md`, `**/docs/*{TOKEN}_TODO.md` (paired docs) + +## Step 3 — Execute the Glob and dedupe against already-loaded docs + +Run each glob pattern via the host agent's Glob tool. Collect all matching absolute paths. + +**Dedupe against `[LOADED_DOCS: ...]` prefix:** +- If a match is already in LOADED_DOCS → skip it (Rule #3) +- If a match is under `bin/`, `obj/`, `node_modules/`, `Test_Benchmark_Results/`, or a worktree-backup path → skip it (not framework docs) + +If the total match count exceeds 10, narrow the glob pattern (e.g., require domain token near the filename start, not just substring). LLM context is finite. + +## Step 4 — Load the filtered set + +Read all remaining matches in parallel (batch the Read calls in one tool-use block). The newly-loaded basenames will appear in your next response's `[LOADED_DOCS: ...]` prefix under the `+K this turn: ` delta, per the active repo's Rule #1 format. + +## Step 5 — Respect the paired-docs convention + +If any `{DOMAIN}.md` is loaded (e.g., `LOGGING.md`), ALSO glob and load its companions: + +- `{DOMAIN}_ISSUES.md` — known issues / limitations / workarounds +- `{DOMAIN}_TODO.md` — planned work / open tickets + +These are **paired docs** and must be loaded as a set. Skipping ISSUES/TODO risks reintroducing fixed bugs or conflicting with ongoing refactors. + +## Step 6 — Proceed to the user's task + +The response's `[LOADED_DOCS: N files (+K this turn: )]` prefix (per the active repo's Rule #1) already surfaces the newly-loaded filenames and the cumulative count. **No separate confirmation line is needed** — the prefix itself is the confirmation. Continue directly to the user's actual request. + +If any relevant docs were skipped as already-loaded (Rule #3 dedupe), you MAY optionally mention them inline where relevant (e.g., "I already have LOGGING.md from earlier"). Do not reiterate the full loaded list. + +## Do NOT + +- **Re-read** any `.md` file already in `[LOADED_DOCS: ...]` — the **no-re-read** rule is absolute (check the active repo's `copilot-instructions.md` for the authoritative phrasing; rule number may differ per repo). The only exception: user explicitly states the file has changed on disk via external means. +- **Load unrelated domains** — if the user asks about the Logger, don't load SignalR docs "just in case". +- **Load more than ~10 files** in a single invocation — if the glob matches more, refine the pattern. If the request truly spans many domains, split into multiple sequential invocations with narrower scope each. +- **Skip folder `README.md`** — if the active repo's conventions include a **folder-navigation / folder-README-first** rule, honour it. `README.md` in a loaded `docs/` folder is always in scope. + +## Tool usage + +This skill is tool-neutral. Map these capabilities to the host agent's tools (per the active repo's `CLAUDE.md`): + +- Globbing file paths: `Glob` (Claude Code), `file_search` (Copilot), `Get-ChildItem -Filter` +- Reading files: `Read` (Claude Code), `get_file` (Copilot) +- Parallelizing reads: issue multiple tool calls in a single response where the host supports it + +## Edge cases + +- **No matching docs found:** Emit `> docs-discovery: no .md matches for tokens [list]. Proceeding with code-search only.` This is informational — the task may be in a domain without documentation, which is itself a signal to be careful. +- **Token extraction is ambiguous:** Prefer SUPERSET — load a few extra .md files rather than missing relevant ones. Loading 3 extra docs is cheap; missing ISSUES.md and reintroducing a fixed bug is expensive. +- **User says "don't load docs" / "just search the code":** Respect it. Skip this skill entirely for that turn. +- **Recursive trigger:** If loaded docs reference other `.md` files via cross-reference, do NOT auto-follow unless the user's request explicitly extends to them. Cross-refs can cascade; relevance-bounded glob is the primary mechanism. diff --git a/.github/skills/protocol-audit/SKILL.md b/.github/skills/protocol-audit/SKILL.md index 8d0b4a7..ae8c18a 100644 --- a/.github/skills/protocol-audit/SKILL.md +++ b/.github/skills/protocol-audit/SKILL.md @@ -1,111 +1,157 @@ --- name: protocol-audit -description: Audit the 5 AyCode/Mango repos (AyCode.Core, AyCode.Blazor, Libraries, FruitBank, FruitBankHybridApp) for .github/copilot-instructions.md protocol consistency. Checks Rule numbering, required substrings (NO-RE-READ definition, auto-detection triggers, CROSS-REPO HARD-GATE, PER-QUESTION DOC-FIRST, Rule 5 scope), @repo block format, and own-dep-repos relative paths. Use when the user asks to "audit protocol", "check instruction consistency", "verify repo rules", "check cross-repo drift", or after modifying the AI AGENT CORE PROTOCOL in any of the 5 repos. Produces a repo x invariant table and concrete patch suggestions; does NOT modify any file without explicit consent. -compatibility: Designed for Claude Code and GitHub Copilot (VS). Requires read access to the 5 listed repo paths. +description: Audit the 8 AyCode/Mango `.github/copilot-instructions.md` files for protocol consistency. Two file types are recognized — **primary** (5 files with full numbered AI AGENT CORE PROTOCOL: AyCode.Core, AyCode.Blazor, Libraries, FruitBank, FruitBankHybridApp) and **inherit** (3 files that reference AyCode.Core's protocol: Mango.Nop.Core, Nop.Plugin.Misc.AIPlugin, Mango.FruitBank). The skill applies the appropriate invariant set per type. Use when the user asks to "audit protocol", "check instruction consistency", "verify repo rules", "check cross-repo drift", or after modifying the AI AGENT CORE PROTOCOL in any repo. Produces a per-file × invariant table with concrete patch suggestions; does NOT modify any file without explicit consent. +compatibility: Designed for Claude Code and GitHub Copilot (VS). Requires read access to the 8 paths listed in `references/REPOS.md`. metadata: author: Fullepi - version: "1.0" + version: "2.0" --- # Protocol Audit -Verify that the 5 AyCode/Mango repos share a consistent AI AGENT CORE PROTOCOL in `.github/copilot-instructions.md`. +Verify that all 8 known `.github/copilot-instructions.md` files share a consistent AI AGENT CORE PROTOCOL ecosystem. **Primary** files contain the full numbered protocol; **inherit** files reference AyCode.Core's protocol without duplicating the numbered rules. ## Before you start -This skill READS files and REPORTS findings. It MUST NOT modify any file. Patch suggestions are surfaced as diffs for the user to review and approve. Follow Rule #5 from the active repo's `copilot-instructions.md`. +This skill READS files and REPORTS findings. It MUST NOT modify any file. Patch suggestions are surfaced as diffs for the user to review and approve. Follow Rule #5 (or equivalent) from the active repo's `copilot-instructions.md`. ## Step 1 — Load the repo list -Read `references/REPOS.md` (relative to this SKILL.md). Extract the 5 absolute repo paths and the expected own-dep-repos table. +Read `references/REPOS.md` (relative to this SKILL.md). Extract: +- **Primary files table** (rows 1-5) with absolute paths +- **Inherit files table** (rows 6-8) with absolute paths +- **Expected own-dep-repos** tables (one per file type) +- **Known issues** section (pre-flagged expected failures) ## Step 2 — Read each instruction file -For each repo in REPOS.md, read `\.github\copilot-instructions.md` once. Record the full text keyed by repo name. +For each entry in both tables, read `\.github\copilot-instructions.md` once. Record the full text keyed by the logical name and **file type** (primary / inherit). If a file is empty (0 bytes), record as `EMPTY` and still run the size-sensitive invariants (expected: all fail). Do NOT re-read a file that is already in your LOADED_DOCS list (per Rule #3 of the active repo's protocol). -## Step 3 — Run invariant checks +## Step 3 — Run invariant checks by file type -For each file, evaluate these invariants. Each yields PASS / FAIL and (on FAIL) an evidence snippet. +Each invariant yields `PASS` / `FAIL` / `N/A` (not-applicable-to-this-type) and, on FAIL, an evidence snippet. -### Structural invariants +### 3A — Common invariants (applied to ALL 8 files) -1. **Rule numbering is contiguous 1..N** - Extract all `^\d+\. \*\*` lines. The numbers must form `1, 2, 3, ..., N` with no gaps and no duplicates. +**C1. `@repo` block has all 4 required fields** +Inside the `@repo { ... }` block, the keys `name`, `type`, `layer`, `own-dep-repos` must all be present. -2. **Rule count is >= 5** - The AI AGENT CORE PROTOCOL has five core rules. Sections after rule 5 (Conventions etc.) may add more; the first 5 are mandatory. +**C2. `own-dep-repos` paths resolve to existing directories** +For each `": "` entry, resolve `/` and check the directory exists. -### Rule-content invariants +**C3. `own-dep-repos` matches REPOS.md expectations** +The dep set must equal the expected set in REPOS.md's relevant "Expected own-dep-repos" table. -3. **Rule #2 contains `CROSS-REPO HARD-GATE`** - Substring present. +### 3B — Primary-only invariants (applied to rows 1-5 in REPOS.md) -4. **Rule #2 contains `PER-QUESTION DOC-FIRST`** - Substring present. +**P1. Rule numbering is contiguous 1..N** +Extract all `^\d+\. \*\*` lines. Numbers must form `1, 2, 3, ..., N` with no gaps and no duplicates. -5. **Rule #3 is the NO-RE-READ rule** - Header matches `STRICT NO-RE-READ POLICY (ANTI-LOOP)`. +**P2. Rule count is ≥ 5** +The AI AGENT CORE PROTOCOL has five core rules. Sections after Rule #5 (Conventions etc.) may add more; the first 5 are mandatory. -6. **Rule #3 contains the "in context" definition** - Substring `lossy compressions` present. +**P3. Rule #1 uses count+delta format** +Substring `N files (+K this turn` present in Rule #1. Old substring `comma-separated list of .md files currently in your context` must be ABSENT. -7. **Rule #4 contains auto-detection triggers** - Substring `Auto-detection triggers` present AND substring `LOADED_DOCS: NONE` present. +**P4. Rule #2 contains `CROSS-REPO HARD-GATE`** -8. **Rule #5 scope is broad** - Substring `any file (code, documentation, configuration, memory, or otherwise)` present. - Negative check: substring `delete code/files without` must be ABSENT (old wording). +**P5. Rule #2 contains `PER-QUESTION DOC-FIRST`** -### Cross-reference invariants +**P6. Rule #3 is the NO-RE-READ rule** +Header matches `STRICT NO-RE-READ POLICY (ANTI-LOOP)`. -9. **"strictly maintain rule 3" reference exists in Rule #2** - Substring `strictly maintain rule 3` present. Old references (`rule 15`, `rule 18`, `rule 19`, `rule 20`, `rule 21`) must be absent. +**P7. Rule #3 contains the "in context" definition** +Substring `lossy compressions` present. -### @repo block invariants +**P8. Rule #4 contains auto-detection triggers** +Substring `Auto-detection triggers` present AND substring `LOADED_DOCS: NONE` present. -10. **@repo block has all 4 required fields** - Inside the `@repo { ... }` block, the keys `name`, `type`, `layer`, `own-dep-repos` must all be present. +**P9. Rule #5 scope is broad** +Substring `any file (code, documentation, configuration, memory, or otherwise)` present. +Negative: substring `delete code/files without` must be ABSENT (old wording). -11. **own-dep-repos paths resolve to existing directories** - For each `": "` entry, resolve `/` and check the directory exists. +**P10. `strictly maintain rule 3` reference exists** +Substring `strictly maintain rule 3` present. Old references (`rule 15`, `rule 18`, `rule 19`, `rule 20`, `rule 21`) must be absent. -12. **own-dep-repos matches REPOS.md expectations** - For each repo, the deps listed must equal the expected set in REPOS.md's "Expected own-dep-repos" table. +### 3C — Inherit-only invariants (applied to rows 6-8 in REPOS.md) + +**I1. References AyCode.Core's protocol** +Substring `follows the AI Agent Core Protocol defined in AyCode.Core` present. + +**I2. Does NOT duplicate numbered Rules #1-5** +Must NOT contain the header `🛑 AI AGENT CORE PROTOCOL (CRITICAL ENFORCEMENT)` (that belongs to primary files only). If the file has `^\d+\. \*\*MANDATORY OUTPUT PREFIX` or similar, flag as FAIL — the inherit file has leaked primary content. + +**I3. Has a link to the Decision Log** +Substring `LLM_PROTOCOL_DECISIONS.md` present (via the Protocol History section — see X2 below). + +### 3D — Cross-cutting invariants (applied to 4 non-Core primary + 3 inherit = 7 files) + +These invariants apply to files OTHER than AyCode.Core itself — AyCode.Core does not need to reference itself. + +**X1. `## Shared Agent Skills` section present** +Header `## Shared Agent Skills` must appear. Both `docs-discovery` and `protocol-audit` bullets must be listed under it. + +**X2. `## Protocol History` section present** +Header `## Protocol History` must appear AND it must reference `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md`. + +### Invariant applicability matrix + +| Invariant set | AyCode.Core | 4× non-Core primary | 3× inherit | +|---------------|-------------|---------------------|------------| +| Common (C1-C3) | ✓ | ✓ | ✓ | +| Primary-only (P1-P10) | ✓ | ✓ | N/A (skip) | +| Inherit-only (I1-I3) | N/A (skip) | N/A (skip) | ✓ | +| Cross-cutting (X1-X2) | N/A (skip) | ✓ | ✓ | + +Use `N/A` in the report cell, not `PASS`, for skipped invariants — so it's obvious the check wasn't applicable. ## Step 4 — Produce the report -Emit a markdown table with one row per repo and one column per invariant. Cell values: `PASS` / `FAIL`. Use concise column headers (e.g., `1.Num`, `2.Cnt`, `3.XR-HG`, ...). +Emit a markdown report with three sections: -Below the table, list every FAIL with: -- Repo name -- Invariant ID -- Evidence (the offending line or missing substring) -- Suggested patch (concrete old-string / new-string pair the user can review) +### 4A — Summary table + +One row per file, grouped by type. Columns: the invariant IDs from Step 3 (C1, C2, C3, P1..P10, I1..I3, X1, X2). Cell values: `PASS` / `FAIL` / `N/A` / `MISSING` / `UNREADABLE` / `EMPTY`. + +Use short column headers (C1, P1, P2, I1, X1, etc.) — 19 columns is dense but fits in a readable table when values are 2-4 chars. + +### 4B — Failure details + +For every FAIL, list: +- File name + type (primary / inherit) +- Invariant ID (e.g., P3, I1, X2) +- Evidence — the offending line, missing substring, or unresolved path +- Suggested patch — a concrete `old_string` / `new_string` pair (or "create this section" scaffold) the user can review + +### 4C — Known-issues reconciliation + +Cross-reference any FAIL with the "Known issues" section in REPOS.md. If a failure is already tracked as a known issue, mark it `FAIL (known)` so the user can distinguish fresh regressions from pre-existing TODOs. ## Step 5 — DO NOT apply patches End the report with: -> All checks complete. N failures detected. To apply any of the suggested patches, reply with "apply patches 1,2,5" (or similar). No files have been modified. +> All checks complete. N failures detected (M known, N-M new). To apply any of the suggested patches, reply with "apply patches P3, I1" (or similar IDs). No files have been modified. -Wait for explicit user consent before using any edit/write tool. +Wait for **explicit** user consent before using any edit / write tool. Per Rule #5: phrases like "we are just thinking" / "what do you think" do NOT constitute approval. ## Tool usage -This skill is tool-neutral. Use whichever file-reading and file-editing tool your host agent provides: +This skill is tool-neutral. Map these capabilities to the host agent's tools (per the active repo's `CLAUDE.md`): - Reading files: `Read` (Claude Code), `get_file` (Copilot), or equivalent -- Directory existence checks: `Glob` / `file_search` / `ls` / `Test-Path` -- Applying patches (only after consent): `Edit` / `replace_string_in_file` - -Map these to your tools per the active repo's `CLAUDE.md` (if present) before proceeding. +- Globbing / directory existence: `Glob`, `file_search`, `ls`, `Test-Path` +- Applying patches (only after consent): `Edit`, `replace_string_in_file` ## Edge cases -- **Repo path missing from disk:** Skip that repo, record as `MISSING` in every invariant cell, and continue with the others. -- **File read fails:** Same handling — record `UNREADABLE`. +- **Repo path missing from disk:** Skip that file, record `MISSING` in every invariant cell, continue with the others. +- **File read fails:** Record `UNREADABLE`, continue. +- **File is 0 bytes:** Record `EMPTY`; every content-sensitive invariant returns FAIL. Still run C1-C3 (they'll fail too, which is correct signal). - **Multiple `@repo` blocks in one file:** Audit the first one; flag the duplicate as its own finding. -- **Rule order differs (e.g., Rule #3 and #4 swapped):** Invariants 5 and 7 fail independently — do not try to auto-reorder. -- **User is running this mid-edit:** If a file has obviously unfinished edits (e.g., truncated mid-sentence), report as `CORRUPT` and stop that repo's audit. +- **Rule order differs** (e.g., Rules #3 and #4 swapped in a primary file): invariants P6 and P8 fail independently — do not try to auto-reorder. +- **Unfinished mid-edit:** If a file has obviously truncated content (cut off mid-sentence), record `CORRUPT` and stop that file's audit. +- **Ambiguous file type** (has both `AI AGENT CORE PROTOCOL` header AND `follows the AI Agent Core Protocol defined in AyCode.Core` blockquote): flag as FAIL on I2 and P1 — file has structural identity crisis, user must resolve. +- **New file not in REPOS.md:** Do NOT audit it automatically. Report it separately as `"Unregistered .github/copilot-instructions.md found at — add to REPOS.md to include in future audits."` diff --git a/.github/skills/protocol-audit/references/REPOS.md b/.github/skills/protocol-audit/references/REPOS.md index ee7082a..1ba18aa 100644 --- a/.github/skills/protocol-audit/references/REPOS.md +++ b/.github/skills/protocol-audit/references/REPOS.md @@ -1,25 +1,75 @@ # Repos under protocol-audit -Each row is one repo whose `.github/copilot-instructions.md` must obey the shared AI AGENT CORE PROTOCOL. +Each row is one `.github/copilot-instructions.md` file that participates in the shared AI AGENT CORE PROTOCOL — either as **Primary** (full numbered-rule protocol) or **Inherit** (references AyCode.Core's protocol, minimal own content). + +## Primary protocol files + +These files contain the full numbered AI AGENT CORE PROTOCOL (Rules #1-5), `@repo` block, and Conventions. All structural, rule-content, and cross-reference invariants apply. | # | Name | Absolute path | Layer | |---|---------------------|----------------------------------------------------------------------------|------------------| -| 1 | AyCode.Core | `H:\Applications\Aycode\Source\AyCode.Core` | framework | -| 2 | AyCode.Blazor | `H:\Applications\Aycode\Source\AyCode.Blazor` | framework | -| 3 | Libraries | `H:\Applications\Mango\Source\NopCommerce.Common\4.70\Libraries` | shared libraries | -| 4 | FruitBank | `H:\Applications\Mango\Source\FruitBank` | product | -| 5 | FruitBankHybridApp | `H:\Applications\Mango\Source\FruitBankHybridApp` | product | +| 1 | AyCode.Core | `H:\Applications\Aycode\Source\AyCode.Core` | framework (0) | +| 2 | AyCode.Blazor | `H:\Applications\Aycode\Source\AyCode.Blazor` | framework (1) | +| 3 | Libraries | `H:\Applications\Mango\Source\NopCommerce.Common\4.70\Libraries` | shared libs (2) | +| 4 | FruitBank | `H:\Applications\Mango\Source\FruitBank` | product (3) | +| 5 | FruitBankHybridApp | `H:\Applications\Mango\Source\FruitBankHybridApp` | product (3) | + +## Inherit protocol files + +These files reference AyCode.Core's protocol via blockquote and do NOT repeat the numbered rules. Only the reduced invariant set applies (see "Invariants by type" below). + +| # | Name | Absolute path | Layer | Role | +|---|----------------------------|------------------------------------------------------------------------------------------------|------------------|-------------------------------------------| +| 6 | Mango.Nop.Core | `H:\Applications\Mango\Source\NopCommerce.Common\4.70\Libraries\Mango.Nop.Core` | framework (2) | Domain framework for NopCommerce plugins | +| 7 | Nop.Plugin.Misc.AIPlugin | `H:\Applications\Mango\Source\NopCommerce.Common\4.70\Plugins\Nop.Plugin.Misc.AIPlugin` | consumer (3) | FruitBank plugin source code | +| 8 | Mango.FruitBank | `H:\Applications\Mango\FruitBank` | host (4) | nopCommerce deployment for FruitBank co. | The instruction file to audit for each is: `\.github\copilot-instructions.md`. ## Expected own-dep-repos (for @repo block path validation) -| Repo | Expected deps (relative path from this repo's root) | -|--------------------|-------------------------------------------------------------------------------------------------------------| -| AyCode.Core | — (none) | -| AyCode.Blazor | `AyCode.Core: ../AyCode.Core` | -| Libraries | `AyCode.Core: ../../../../../Aycode/Source/AyCode.Core` | -| FruitBank | `AyCode.Core: ../../../Aycode/Source/AyCode.Core`, `Libraries: ../NopCommerce.Common/4.70/Libraries` | +### Primary + +| Repo | Expected deps (relative path from this repo's root) | +|--------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------| +| AyCode.Core | — (none) | +| AyCode.Blazor | `AyCode.Core: ../AyCode.Core` | +| Libraries | `AyCode.Core: ../../../../../Aycode/Source/AyCode.Core` | +| FruitBank | `AyCode.Core: ../../../Aycode/Source/AyCode.Core`, `Libraries: ../NopCommerce.Common/4.70/Libraries` | | FruitBankHybridApp | `AyCode.Core: ../../../Aycode/Source/AyCode.Core`, `AyCode.Blazor: ../../../Aycode/Source/AyCode.Blazor`, `Libraries: ../NopCommerce.Common/4.70/Libraries` | -When modifying the repo set (add/remove/move), update this table. SKILL.md remains stable. +### Inherit + +| Repo | Expected deps (relative path from this repo's root) | +|---------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------| +| Mango.Nop.Core | `AyCode.Core: ../../../../../../Aycode/Source/AyCode.Core` | +| Nop.Plugin.Misc.AIPlugin | `Mango.Nop.Core: ../../Libraries/Mango.Nop.Core`, `AyCode.Core: ../../../../../../Aycode/Source/AyCode.Core` | +| Mango.FruitBank | `AyCode.Core: ../../Aycode/Source/AyCode.Core` | + +## Invariants by type + +**Primary files (1-5)** — full invariant set per `SKILL.md`: +- Rule numbering contiguous 1..N +- Rule #2 contains `CROSS-REPO HARD-GATE` and `PER-QUESTION DOC-FIRST` +- Rule #3 is `STRICT NO-RE-READ POLICY (ANTI-LOOP)` and contains "in context" definition (`lossy compressions`) +- Rule #4 contains auto-detection triggers +- Rule #5 contains broad scope wording (`any file (code, documentation, configuration, memory, or otherwise)`) +- `strictly maintain rule 3` reference exists +- `## Shared Agent Skills` section with both skills listed +- `## Protocol History` section referencing `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md` +- `@repo` block has all 4 required fields, paths resolve to existing directories + +**Inherit files (6-8)** — reduced invariant set: +- References AyCode.Core's protocol via substring: `follows the AI Agent Core Protocol defined in AyCode.Core` +- Has `## Shared Agent Skills` section listing **both** skills (docs-discovery + protocol-audit) +- Has `## Protocol History` section referencing `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md` +- `@repo` block (if present) has all 4 required fields, paths resolve to existing directories +- Numbered rules are NOT required (they are inherited from AyCode.Core) + +## Known issues + +*(No open issues. Previously tracked — Mango.Nop.Core path off-by-one and Mango.FruitBank purpose TBD — are resolved; see `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md` for the fix history.)* + +## Maintenance note + +When modifying the repo set (add, remove, or relocate), update this table. `SKILL.md` remains stable because it reads this file at runtime. diff --git a/AyCode.Core/docs/BINARY_ISSUES.md b/AyCode.Core/docs/BINARY_ISSUES.md index 3ac2046..1f34921 100644 --- a/AyCode.Core/docs/BINARY_ISSUES.md +++ b/AyCode.Core/docs/BINARY_ISSUES.md @@ -83,3 +83,51 @@ Same constraint as SER-2 — `IBinaryInputBase` interface is synchronous. `ReadA **Affects:** Generated reader code The source generator emits `null` assignments for non-nullable reference type properties during deserialization (before the value is read from the stream). This produces CS8625 warnings. Functionally harmless — the property is always assigned before use. + +## Buffer Writer (BWO) + +### BWO-1: Struct copy semantics + +**Status:** By design +**Affects:** `BufferWriterBinaryOutput` value-type assignment + +Assigning a `BufferWriterBinaryOutput` value creates an independent copy. State changes (e.g. `_committedBytes` via `Grow`/`Flush`) are not reflected in the original. Copy back after use if needed. + +### BWO-2: Initialize resets tracking + +**Status:** By design +**Affects:** `BufferWriterBinaryOutput.Initialize` (context mode) + +`Initialize` sets `_committedBytes = 0`. Standalone bytes written before are lost if the BWO is then passed to a context. Call `FlushAndReset()` first, or track standalone bytes separately. + +### BWO-3: Constructor acquires chunk + +**Status:** Acceptable (not a leak) +**Affects:** `BufferWriterBinaryOutput` ctor + +`AcquireChunk` runs in ctor for standalone readiness. Redundant if only context mode is used (context `Initialize` acquires its own). Not a leak — consecutive `GetMemory` without `Advance` returns overlapping memory. + +### BWO-4: No mode mixing + +**Status:** By design +**Affects:** `BufferWriterBinaryOutput` — context vs standalone mode + +A single instance must not use context + standalone modes simultaneously — buffer states desynchronize. One mode per lifecycle phase; `FlushAndReset()` as boundary between modes. + +### SGEN-2: Consumer entity with `new` Id shadowing — excluded from SGen + +**Status:** Workaround-in-place (compiled-expression fallback) +**Affects:** Any consumer entity whose base class hides `BaseEntity.Id` with `readonly new int Id { get; }` pattern (e.g. `DiscountProductMapping` in Mango.Nop.Core) + +When the base class shadows `Id` with a setter-less `new int Id { get; }`, SGen can't emit a setter without CS0200. Runtime falls back to compiled-expression serialization for these types. Low priority — affects a small number of consumer entities. + +**Related TODO:** `BINARY_TODO.md#todo-02` + +## Cross-cutting (also tracked in SignalR-side docs) + +### XCUT-1: JSON-in-Binary request parameters + +**Status:** Major tech debt, planned replacement (coordinated) +**Affects:** SignalR client→server request parameter path + +Request parameters currently use JSON inside a Binary envelope (`SignalPostJsonDataMessage`). Response path already uses pure Binary. Planned migration is a cross-project coordinated change — see `AyCode.Services/docs/SIGNALR_ISSUES.md` PROTO section and `BINARY_TODO.md#todo-01` for the broader picture. Do NOT attempt as a side-effect of unrelated work. diff --git a/AyCode.Core/docs/BINARY_TODO.md b/AyCode.Core/docs/BINARY_TODO.md new file mode 100644 index 0000000..cedcb83 --- /dev/null +++ b/AyCode.Core/docs/BINARY_TODO.md @@ -0,0 +1,18 @@ +# AcBinarySerializer — TODO + +## Priority legend +- **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea + +--- + +## TODO-01: Replace JSON-in-Binary request parameters +**Priority:** P1 · **Type:** Refactor · **Related:** `BINARY_ISSUES.md#issue-01`, `AyCode.Services/docs/SIGNALR_TODO.md` + +Migrate client→server request parameters from JSON-in-Binary envelope to direct Binary serialization (matching response path). Coordinated change across client, server, and all consuming projects. Do NOT attempt as side-effect of unrelated work. + +**Acceptance:** `SignalPostJsonDataMessage` replaced by a `SignalPostBinaryDataMessage` (or equivalent); no JSON round-trip on the wire for request params; benchmarks confirm no regression. + +## TODO-02: Re-evaluate DiscountProductMapping SGen exclusion +**Priority:** P3 · **Type:** Investigation · **Related:** `BINARY_ISSUES.md#issue-02` + +Investigate whether the `new int Id` shadowing pattern can be handled by SGen (via base-class introspection, property-setter lookup on the base) to eliminate the runtime compiled-expression fallback for this entity class. diff --git a/AyCode.Core/docs/BINARY_WRITERS.md b/AyCode.Core/docs/BINARY_WRITERS.md index 6f83ef4..592e9e6 100644 --- a/AyCode.Core/docs/BINARY_WRITERS.md +++ b/AyCode.Core/docs/BINARY_WRITERS.md @@ -61,10 +61,7 @@ Context/standalone share only `IBufferWriter` ref and `_committedBytes`. ### Known Limitations -1. **Struct copy semantics:** value type → assignment creates independent copy. State changes (e.g. `_committedBytes` via `Grow`/`Flush`) not reflected in original. Copy back after use if needed. -2. **Initialize resets tracking:** `_committedBytes = 0`. Standalone bytes lost if BWO then passed to context. Use `FlushAndReset()` before, track standalone bytes separately. -3. **Constructor acquires chunk:** `AcquireChunk` in ctor for standalone readiness. Redundant if only context mode used (context `Initialize` acquires its own). Not a leak — consecutive `GetMemory` without `Advance` returns overlapping memory. -4. **No mode mixing:** single instance must not use context+standalone simultaneously. Buffer states desynchronize. One mode per lifecycle phase, `FlushAndReset()` as boundary. +Buffer-writer contract limitations are tracked in `BINARY_ISSUES.md` under the **Buffer Writer (BWO)** category — struct copy semantics, init-reset tracking, ctor chunk acquisition, no-mode-mixing rule. ### Chunk Size diff --git a/AyCode.Core/docs/LOGGING.md b/AyCode.Core/docs/LOGGING.md index af50590..d27fa21 100644 --- a/AyCode.Core/docs/LOGGING.md +++ b/AyCode.Core/docs/LOGGING.md @@ -118,6 +118,89 @@ Each writer's constructor signature must accept `(AppType, LogLevel, string?)`. `AcLogWriterBase` also reads its own `LogLevel` from config by matching its `AssemblyQualifiedName` against the `LogWriterType` entries. This means a writer instantiated manually (not via config) can still pick up config values if a matching entry exists. +## DI-Based Factory Pattern + +Modern, framework-first alternative to the config-reading `AcLoggerBase(string)` ctor. No runtime reflection over writer config; concrete writer types resolved from DI. Recommended for all modern projects (MAUI, WASM, ASP.NET Core) — the config-reading path is filesystem-bound and unsuitable for MAUI/WASM (see `LOGGING_ISSUES.md#issue-02`). + +### Consumer setup in Program.cs + +```csharp +// 1. Bind options from appsettings.json +services.Configure(configuration.GetSection("AyCode:Logger")); + +// 2. Register writer(s) as DI singletons +services.AddSingleton(); +// Or, for client-scoped marker: +// services.AddSingleton(); + +// 3. Register the logger factory — registers Func singleton in DI +services.AddAcLoggerFactory(); +// Or, with custom writer marker (pulls only TWriterBase-registered writers): +// services.AddAcLoggerFactory(); +``` + +Consumers inject `Func` and invoke it with a category name to obtain category-scoped logger instances. + +### Appsettings.json shape + +```json +{ + "AyCode": { + "Logger": { + "AppType": "Web", + "LogLevel": "Debug" + } + } +} +``` + +Only `AppType` and `LogLevel` bind to `AcLoggerOptions`. The legacy `LogWriters[]` array used by the config-reading ctor is independent and unused in this pattern. + +### AcLoggerOptions + +| Property | Type | Default | Purpose | +|----------|------|---------|---------| +| `AppType` | `AppType` | `Server` | Stamped on each log entry | +| `LogLevel` | `LogLevel` | `Info` | Global minimum level (Logger's own gate) | + +Extend the POCO as needed; consumer's `services.Configure` binding picks up new properties automatically. + +### TLogger ctor requirement + +`AddAcLoggerFactory` uses `Activator.CreateInstance(typeof(TLogger), AppType, LogLevel, categoryName, writers)`. `TLogger` must expose a public constructor with signature: +```csharp +public TLogger(AppType appType, LogLevel logLevel, string? categoryName, params IAcLogWriterBase[] logWriters) +``` + +If your concrete logger doesn't have this signature, register a manual factory instead: +```csharp +services.AddSingleton>(sp => category => new MyConcreteLogger(...)); +``` + +### Writer-marker scoping (two overloads) + +- `AddAcLoggerFactory()` — all `IAcLogWriterBase`-registered writers go into the logger +- `AddAcLoggerFactory()` — only writers registered as `TWriterBase` (e.g. `IAcLogWriterClientBase` for client-only) + +Use the two-arg overload when the consumer separates client-only and server-only writers via a marker interface. + +### Companion extension: AddAcDefaults (SignalR) + +For SignalR client setup that wires the same `AcLoggerBase` instance into Microsoft.Extensions.Logging, use `AcSignalRConnectionExtensions.AddAcDefaults(builder, logger, connectionOptions)` — it bundles `AddAcConnection` + `ConfigureLogging(AddAcLogger)` in a single call. See `AyCode.Services/docs/SIGNALR.md`. + +### Comparison: config-reading ctor vs DI-based factory + +| Aspect | Config-reading ctor | DI-based factory | +|--------|---------------------|------------------| +| Writer instantiation | `Activator.CreateInstance` per writer entry | Resolved from DI singletons | +| Writer config location | `AyCode:Logger:LogWriters[]` | `services.AddSingleton()` | +| Writer ctor requirement | `(AppType, LogLevel, string?)` | Consumer-defined (via DI) | +| Writer lifecycle | Per-logger instance | Singleton, shared across loggers | +| Typical use | Legacy / server-side config-driven | Modern DI: MAUI, WASM, ASP.NET Core | +| MAUI/WASM-safe | ❌ (filesystem-bound `AcEnv.AppConfiguration`) | ✅ | + +Both patterns can coexist — consumer picks per scenario. Related issues: `LOGGING_ISSUES.md`. Planned unification: `LOGGING_TODO.md#todo-03`. + ## Core Components ### AcLoggerBase diff --git a/AyCode.Core/docs/LOGGING_ISSUES.md b/AyCode.Core/docs/LOGGING_ISSUES.md new file mode 100644 index 0000000..8af7335 --- /dev/null +++ b/AyCode.Core/docs/LOGGING_ISSUES.md @@ -0,0 +1,155 @@ +# Logger — Known Issues + +For planned/actionable work see `LOGGING_TODO.md`. + +## ISSUE-01: NopLogWriter ctor signature mismatch (consumer-specific but framework-exposed) + +**Severity:** Minor (caught, non-blocking, but noisy) · **Status:** Open · **Area:** Writer-instantiation contract (`AcLoggerBase(string)` config-reading ctor) + +### Description +When `AcLoggerBase(string? categoryName)` (config-reading ctor) runs, it iterates `AyCode:Logger:LogWriters[]` and calls `Activator.CreateInstance(writerType, AppType, LogLevel, categoryName)`. Some consumer writers (notably a NopCommerce-plugin-level `NopLogWriter`) have DI-injected ctors instead (e.g. `(INopLoggerMsSqlNopDataProvider, CommonSettings, CustomerSettings, IWebHelper, ILogger, string?)`), so no ctor matches → `MissingMethodException`. + +Exception is caught in `AcLoggerBase` try/catch, logged to `Console.Error`, loop continues with other writers. Non-blocking — protocol construction succeeds — but produces noisy startup logs per `Logger` instance. + +### Root cause +Two logger-construction paradigms coexist: +- Config-reading ctor expects `(AppType, LogLevel, string?)` writer ctors for `Activator` convenience +- DI-injected writers have arbitrary ctor signatures + +### Known workaround +Console.Error noise tolerated. Alternatively, consumer uses DI-based `AddAcLoggerFactory` (see LOGGING.md) instead of the config-reading ctor — this path doesn't touch `LogWriters[]`. + +### Related TODO +`LOGGING_TODO.md#todo-01` + +## ISSUE-02: AcEnv.AppConfiguration is filesystem-bound, MAUI/WASM-unsafe + +**Severity:** Minor · **Status:** Documented limitation · **Area:** `AyCode.Core.Consts.AcEnv` + +### Description +`AcEnv.AppConfiguration` is a static lazy singleton calling `new ConfigurationBuilder().AddJsonFile("appsettings.json").Build()` on first access. Reads current-working-directory filesystem. Throws on MAUI (no physical appsettings next to exe) and WASM (no filesystem at all). + +### Root cause +Design predates `IOptions`/DI pattern. + +### Known workaround +Consumer avoids the config-reading `AcLoggerBase(string)` ctor on these platforms. DI-based `AddAcLoggerFactory` + `services.Configure(...)` is the replacement (see LOGGING.md). + +### Related TODO +`LOGGING_TODO.md#todo-02` + +## ISSUE-03: Two parallel logger-setup patterns + +**Severity:** Minor (confusion, not functional) · **Status:** Documented · **Area:** LOGGING.md / consumer code + +### Description +Two ways to construct a logger coexist: +1. **Config-reading:** `new Logger(categoryName)` → `AcLoggerBase` reads `AyCode:Logger` section via `AcEnv.AppConfiguration`, instantiates writers via `Activator.CreateInstance` +2. **DI factory:** `services.Configure(...)` + `services.AddAcLoggerFactory()` → `Func` resolved from DI, writers pulled from DI + +Consumer picks per scenario; no automatic bridge. Risk: mixing patterns causes subtle failures (e.g. `MissingMethodException` — see ISSUE-01). + +### Related TODO +`LOGGING_TODO.md#todo-03` + +## ISSUE-04: Default LogLevel diverges across the two setup paths + +**Severity:** Minor (surprise, not broken) · **Status:** Open · **Area:** `AcLoggerBase` field initializer vs `AcLoggerOptions` + +### Description +The two logger-setup paths (see ISSUE-03) ship with **different default LogLevels**: +- `AcLoggerBase.cs:18` — field initializer: `LogLevel = Error` +- `AcLoggerOptions.cs:27` — DI options default: `LogLevel = Info` + +Consequences: +- Config-reading ctor WITHOUT an `AyCode:Logger:LogLevel` entry → silent `Error` minimum (loses Info/Warning in prod) +- DI factory WITHOUT `services.Configure(...)` → `Info` minimum + +Same consumer code, same "no configuration" state, two different log volumes depending on which path was chosen. + +### Root cause +Historical: field initializer predates the Options class; Options was added as part of the DI-factory refactor with a developer-friendly `Info` default. + +### Related TODO +`LOGGING_TODO.md#todo-05` + +## ISSUE-05: `AcConsoleLogWriter.Initialize()` runs twice on parameterless ctor + +**Severity:** Minor (wasted work, not broken) · **Status:** Open · **Area:** `AcConsoleLogWriter` ctor chain + +### Description +`AcConsoleLogWriter.cs:14-17`: +```csharp +protected AcConsoleLogWriter() : this(null) // chains to (string?) ctor +{ + Initialize(); // ← second call +} + +protected AcConsoleLogWriter(string? categoryName = null) : base(categoryName) +{ + Initialize(); // ← first call (via chain) +} +``` + +Chain `: this(null)` invokes the `(string?)` ctor, which calls `Initialize()`. Control returns to the parameterless ctor body and calls `Initialize()` again. Net effect: `Console.ForegroundColor = ConsoleColor.White` set twice — harmless but reveals a copy-paste oversight. + +### Fix direction +Remove the second `Initialize()` call in the parameterless ctor body (the chain already covered it). + +### Related TODO +`LOGGING_TODO.md#todo-06` + +## ISSUE-06: `ILogger.IsEnabled(MsLogLevel.None)` incorrectly reports enabled + +**Severity:** Low (semantic bug, rare path) · **Status:** Open · **Area:** `AcLoggerBase.IsEnabled` + `MapFromMsLogLevel` + +### Description +`AcLoggerBase.cs:184-188`: +```csharp +public bool IsEnabled(MsLogLevel logLevel) +{ + var acLogLevel = MapFromMsLogLevel(logLevel); + return LogLevel <= acLogLevel; +} +``` + +`MapFromMsLogLevel(MsLogLevel.None) → AcLogLevel.Disabled (byte 255)`. For any instance `LogLevel` (max `Error=25`), `LogLevel <= 255` is **always true** → `IsEnabled(None)` returns `true` despite `None` meaning "off". + +The subsequent `Log` switch has `case MsLogLevel.None: default: break;` so no actual write occurs, BUT: +- The MS-logging framework may query `IsEnabled` as a gate before calling `Log` — hosts using that pattern get misleading "enabled" signals +- Any caller that inspects `IsEnabled(None)` (diagnostics, test assertions) gets the wrong answer + +### Fix direction +Either: +- **(a)** Explicit `if (logLevel == MsLogLevel.None) return false;` at the top of `IsEnabled` +- **(b)** Change the comparison to strict `<` with `Disabled` as sentinel (larger refactor — affects semantic of `Disabled` elsewhere) + +### Related TODO +`LOGGING_TODO.md#todo-07` + +## ISSUE-07: Misleading inline comment in `AcLoggerBase.Log` + +**Severity:** Trivial (doc-only) · **Status:** Open · **Area:** `AcLoggerBase.cs:210-211` + +### Description +```csharp +// Use eventId.Name as memberName if available, otherwise null (will show as empty, not "Log") +var memberName = !string.IsNullOrEmpty(eventId.Name) ? $"{eventId.Name}:{eventId.Id}" : "Log"; +``` + +Comment says fallback is `null` (empty display), the code assigns `"Log"`. Contradicts both the doc in `LOGGING.md#CallerMemberName Auto-Capture` and the actual behaviour observed in logs. + +### Fix direction +Update comment to match code: `// Use eventId.Name:eventId.Id if Name is set, otherwise fallback to "Log" per LOGGING.md convention`. + +### Related TODO +Folded into `LOGGING_TODO.md#todo-08` (cleanup batch). + +## Evaluated review findings — NOT bugs (by-design) + +Cross-session review (2026-04) flagged several items that are **intentional defense-in-depth / YAGNI** and should NOT be "fixed": + +- **"Triple LogLevel gate on the ILogger.Log path is redundant"** — NOT redundant. `Info()`/`Debug()` can be called directly (not only via `ILogger.Log`), so each named method MUST own its gate. Per-writer gate is the documented Gate-2 (see LOGGING.md "Two independent level gates"). Removing any gate breaks a real path. +- **"`AcTextLogWriterBase.GetDiagnosticText` defensive level-check is redundant"** — same rationale; direct callers exist. +- **"`IAcLoggerBase` narrow interface (no dynamic LogLevel setter via interface)"** — YAGNI. Consumers needing dynamic level can cast to `AcLoggerBase`. Widening the interface for a rare scenario pollutes the abstraction. +- **"`protected List LogWriters` is mutable"** — acceptable; `GetWriters` returns a defensive copy (`[.. LogWriters]`), direct mutation is only reachable from derived classes (which control their own ctor-time population). diff --git a/AyCode.Core/docs/LOGGING_TODO.md b/AyCode.Core/docs/LOGGING_TODO.md new file mode 100644 index 0000000..28dfd43 --- /dev/null +++ b/AyCode.Core/docs/LOGGING_TODO.md @@ -0,0 +1,85 @@ +# Logger — TODO + +## Priority legend +- **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea + +--- + +## TODO-01: Fix the writer-ctor mismatch for DI-injected writers +**Priority:** P2 · **Type:** Bug fix · **Related:** `LOGGING_ISSUES.md#issue-01` + +Options: +- **A)** Provide a fallback `(AppType, LogLevel, string?)` ctor on consumer writers that have DI-heavy primary ctors, with DI resolution via service-locator +- **B)** Skip writers that don't match the `Activator`-ctor signature, with a single consolidated warning (not per-init spam) +- **C)** Deprecate the config-reading writer-instantiation path entirely in favour of DI registration + +Eliminate the per-startup Console.Error noise regardless. + +## TODO-02: Expose `AcEnv.AppConfiguration` setter for consumer init +**Priority:** P2 · **Type:** Feature · **Related:** `LOGGING_ISSUES.md#issue-02` + +Allow `AcEnv.SetConfiguration(IConfiguration)` so consumer `Program.cs` can do `AcEnv.SetConfiguration(builder.Configuration)` at startup. Enables config-reading pattern on MAUI/WASM without filesystem assumptions. Backward-compat: fall back to filesystem if no explicit set. + +## TODO-03: Unify or clearly separate config-reading and DI-based patterns +**Priority:** P2 · **Type:** Docs / Refactor · **Related:** `LOGGING_ISSUES.md#issue-03` + +Decide the canonical direction: +- **(a)** Deprecate config-reading pattern → all consumers migrate to DI factory +- **(b)** Keep both, with compile-time guidance (analyzer / XML doc `[Obsolete]` hints / decision tree in LOGGING.md) +- **(c)** Merge: DI-factory internally falls back to config-reading when `TLogger` doesn't match the `Activator` ctor shape + +## TODO-04: Per-writer LogLevel via appsettings +**Priority:** P2 · **Type:** Feature + +Extend `AcLoggerOptions` with per-writer LogLevel overrides. Example shape: +```json +"AyCode": { + "Logger": { + "LogLevel": "Info", + "WriterLevels": { "BrowserConsoleLogWriter": "Debug", "SignaRClientLogItemWriter": "Warning" } + } +} +``` +Factory applies overrides when constructing writers. Currently writer-LogLevel is hardcoded in writer ctors. + +## TODO-05: Unify default LogLevel across setup paths +**Priority:** P1 · **Type:** Behaviour decision + cleanup · **Related:** `LOGGING_ISSUES.md#issue-04` + +Decide which default wins (`AcLoggerBase.cs:18` = `Error` vs `AcLoggerOptions.cs:27` = `Info`) and make both paths consistent. Recommendation: **`Info`** — matches the DI/Options path already in use for modern consumers (MAUI, WASM, ASP.NET Core) and is the common developer expectation. + +Change: drop the field initializer in `AcLoggerBase` (or set it to `Info`). Update LOGGING.md with a single "Default LogLevel = Info" line so this can't drift again. + +## TODO-06: Fix `AcConsoleLogWriter.Initialize()` double-run +**Priority:** P3 · **Type:** Bug fix · **Related:** `LOGGING_ISSUES.md#issue-05` + +Remove the redundant `Initialize()` call from the parameterless ctor body — the `: this(null)` chain already invokes it. No behaviour change; removes wasted `Console.ForegroundColor` assignment. + +## TODO-07: Fix `ILogger.IsEnabled(MsLogLevel.None)` wrongly reporting enabled +**Priority:** P2 · **Type:** Bug fix · **Related:** `LOGGING_ISSUES.md#issue-06` + +Add `if (logLevel == MsLogLevel.None) return false;` at the top of `AcLoggerBase.IsEnabled`. One-line fix. Optionally add a unit test covering all six MS LogLevel values → expected bool. + +## TODO-08: Cleanup batch (low-risk micro-refactor, one commit) +**Priority:** P3 · **Type:** Cleanup · **Related:** `LOGGING_ISSUES.md#issue-07` + +Single commit covering: +- Remove unused usings in `AcLoggerBase.cs` (`System.Security.AccessControl`, `System.Net.Mime.MediaTypeNames`) +- Extract the default-category magic string `"..."` (at least 3 occurrences — `AcLoggerBase.cs:36,76`, writer side) into a public const `AcLoggerBase.DefaultCategory = "..."` with XML doc explaining it's the "unknown category" sentinel, NOT a magic string. Update all call-sites. +- Fix misleading comment at `AcLoggerBase.cs:210` — current comment says "fallback null" but code assigns `"Log"`. Make comment match code. +- Decide on the commented-out batch block in `AcLogItemWriterBase.cs:90-119`: either delete (git history preserves) or convert to a single `// TODO: see LOGGING_TODO.md#todo-XX` marker with a new TODO entry for the batch work. + +## TODO-09: Fail-fast ctor validation in `AddAcLoggerFactory` +**Priority:** P2 · **Type:** Feature + +`AcLoggerServiceExtensions.AddAcLoggerFactory` uses `Activator.CreateInstance(typeof(TLogger), AppType, LogLevel, categoryName, writers)` only on first logger resolution. Ctor-mismatch therefore surfaces only at first log call, not at `services.BuildServiceProvider()` time. + +Add a one-time reflection check at registration: `typeof(TLogger).GetConstructor([typeof(AppType), typeof(LogLevel), typeof(string), typeof(IAcLogWriterBase[])])` — if null, throw `InvalidOperationException` with the expected signature in the message. Cost: one reflection call per registration. Benefit: app fails to start (loud) instead of logging being silently broken (quiet). + +## TODO-10: Writer exception isolation +**Priority:** P2 · **Type:** Resilience + +Currently `AcLoggerBase.Info(...)` and friends do `LogWriters.ForEach(x => x.Info(...))`. A throwing writer (e.g. `SignaRClientLogItemWriter` during network blip, `AcDbLogItemWriter` during a DB outage) takes down the fan-out → subsequent writers in the list never see the message. + +Wrap each writer invocation in `try { ... } catch (Exception ex) { Console.Error.WriteLine($"Writer {x.GetType().Name} failed: {ex.Message}"); }`. Optional: escalate to a circuit-breaker pattern (N failures → disable that writer for M seconds) to avoid Console.Error flood on persistent outages. + +Must NOT log to `AcLoggerBase` itself inside the catch (reentrancy / infinite loop risk). diff --git a/AyCode.Services/docs/SIGNALR.md b/AyCode.Services/docs/SIGNALR.md index 4ae8f79..4950517 100644 --- a/AyCode.Services/docs/SIGNALR.md +++ b/AyCode.Services/docs/SIGNALR.md @@ -152,9 +152,7 @@ GetParameterValues(ParameterInfo[]): Type-guided deserialization — each parameter is individually serialized/deserialized with its concrete type, avoiding the `object[]` → dictionary problem of untyped binary deserialization. -**Perf concern:** Per-parameter `ToBinary()`/`BinaryTo(Type)` = N× context pool acquire/release + N× type-dispatch (ThreadLocal + ConcurrentDictionary cache). For many small primitives (int, bool, string) the per-call overhead may exceed a single bulk serialization call. Complex objects benefit clearly. If benchmarks show regression vs old JSON path, a batch fast-path (single serialization context for all params) should be added. - -**Limitation:** Parameter serialization/deserialization is currently AcBinary only (`ToBinary()`/`BinaryTo()`). JSON support would require dispatching on serializer type in `SignalParams` methods + AcJsonSerializer reference. +> Known concerns and limitations on parameter serialization (per-parameter overhead, AcBinary-only) are tracked in `SIGNALR_ISSUES.md` under `PROTO-2` and `PROTO-3`. ## Response Patterns diff --git a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_ISSUES.md b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_ISSUES.md new file mode 100644 index 0000000..fbf306a --- /dev/null +++ b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_ISSUES.md @@ -0,0 +1,34 @@ +# Binary Hub Protocol (wire) — Known Issues + +For planned/actionable work see `SIGNALR_BINARY_PROTOCOL_TODO.md`. +For higher-level SignalR abstractions see `SIGNALR_ISSUES.md`. + +## ISSUE-01: AsyncSegment send-path unsupported on WebAssembly + +**Severity:** Major (on WASM) · **Status:** Workaround-in-place · **Area:** `AsyncPipeWriterOutput` / WASM runtime + +### Description +`AsyncPipeWriterOutput.SyncAwaitFlush` uses `Task.Wait(timeout)` — deadlocks the single-threaded WASM UI thread. Therefore, WASM clients cannot SEND with `AsyncSegment` mode. + +### Known workaround (multi-layer) +1. **`AcBinaryHubProtocolOptions.Validate()`** throws `PlatformNotSupportedException` if WASM + AsyncSegment combination is requested → prevents deadlock +2. **Consumer code-level safety-net** downgrades `AsyncSegment → Segment` on WASM (see `FruitBankHybrid.Web.Client/Program.cs` Configure lambda) +3. **Receive-path** on WASM is fully supported — `SegmentBufferReaderInput` with synchronous fallback at `CHUNK_END` means WASM clients CAN receive AsyncSegment-chunked data from a non-WASM sender, they just cannot send AsyncSegment themselves + +### Related TODO +None — architectural constraint of browser WASM threading model. + +## ISSUE-02: StaticWebAssets SDK "Illegal characters" noise (consumer build) + +**Severity:** Cosmetic (non-blocking) · **Status:** Upstream SDK limitation · **Area:** SDK, not our code + +### Description +Consumer projects using `Microsoft.NET.Sdk.BlazorWebAssembly` may see "Illegal characters in path" errors in the VS design-time error list. Originates from the SDK's `DefineStaticWebAssets` task calling legacy `FileIOPermission.EmulateFileIOPermissionChecks`, which is stricter than NTFS (e.g., double spaces or certain path patterns in any wwwroot asset trigger it). + +Build succeeds, app runs — the "error" is error-list-only. + +### Known workaround +Ignore the error-list entry. If it becomes truly blocking, inspect wwwroot (including transitive from RCL projects) for problematic filenames (double spaces, control chars, reserved chars). + +### Related TODO +None — upstream SDK fix required. diff --git a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_TODO.md b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_TODO.md new file mode 100644 index 0000000..bccf161 --- /dev/null +++ b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL_TODO.md @@ -0,0 +1,46 @@ +# Binary Hub Protocol (wire) — TODO + +## Priority legend +- **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea + +--- + +## TODO-01: SegmentBufferReader isolated unit tests +**Priority:** P1 · **Type:** Test coverage + +Original `vast-brewing-moonbeam` refactor plan (chunked receive-path) listed these tests in its verification section; they were never written. Needed: +- `Write` / `TryAdvanceSegment` / `Complete` basic contract +- Producer-consumer concurrency (grow-buffer handoff race) +- Missed-signal double-check pattern under `ManualResetEventSlim` reset +- `Dispose` lifecycle (buffer pool return, old-buffer cleanup) + +## TODO-02: Chunked protocol integration test +**Priority:** P1 · **Type:** Test coverage + +End-to-end round-trip: +- `WriteMessageChunked(obj, AsyncSegment)` → bytes +- `TryParseMessage(bytes)` → reconstructed object +- Assert structural equality with original + +Cover asymmetric cases: +- AsyncSegment sender, Segment receiver +- Bytes sender, AsyncSegment receiver +- WASM-downgraded sender (Segment), server AsyncSegment receiver + +## TODO-03: `BinaryProtocolMode.Auto` wire-detection implementation +**Priority:** P3 · **Type:** Feature · **Related:** `SIGNALR_TODO.md#todo-04` + +Client-side adaptive mode: on first received message, inspect first payload byte: +- `CHUNK_START (200)` → peer uses AsyncSegment → match it on subsequent sends (subject to local-platform constraint — WASM safety-net overrides to Segment) +- Non-chunked → stay with Segment/Bytes + +Per-`HubConnection` state. Requires changes to `BinaryProtocolMode` enum + detection wiring in `TryParseMessage`. + +## TODO-04: SignalR handshake-extension for upfront mode negotiation +**Priority:** P3 · **Type:** Feature · **Related:** TODO-03 + +Alternative to wire-detection: use SignalR handshake message's `extensions` JSON field to carry protocol-capability info — e.g. +```json +{ "protocol": "acbinary", "version": 1, "extensions": { "acbinary": { "preferredMode": "AsyncSegment" } } } +``` +Zero first-message overhead, fully explicit. Both sides advertise their send-modes; pick intersection. Specification to be drafted; compatibility with non-AC clients (pure JSON etc.) must remain. diff --git a/AyCode.Services/docs/SIGNALR_ISSUES.md b/AyCode.Services/docs/SIGNALR_ISSUES.md index f51c291..2094fd7 100644 --- a/AyCode.Services/docs/SIGNALR_ISSUES.md +++ b/AyCode.Services/docs/SIGNALR_ISSUES.md @@ -53,3 +53,44 @@ Transport max message size (30MB) and application buffer (30MB) are hardcoded. S The `GetAll` path uses `IsRawBytesData = true` to receive raw `byte[]` from the protocol, then deserializes into the existing list via `PopulateMerge`. This avoids allocating a temporary `List` for merge. The extra copy (pipe → byte[]) is the trade-off. **Possible optimization:** Direct typed deserialization with merge support in the deserializer (PopulateMerge from `ReadOnlySequence`). Requires deserializer API changes. + +## Client-side Setup & DI + +### CONN-1: HubConnectionBuilder inner DI isolation + +**Status:** Workaround-in-place (dedicated options-passing overload) +**Affects:** Consumer client setup in `Program.cs` (MAUI, WASM, ASP.NET Core server prerender) + +`HubConnectionBuilder.Services` is a separate `IServiceCollection` from the outer host DI. `services.Configure(...)` registered in the outer container does NOT flow into `HubConnectionBuilder.Services`. Calling `hubBuilder.AddAcBinaryProtocol()` with no args silently falls back to default options. + +**Known workaround:** Use the overload `AddAcBinaryProtocol(IHubConnectionBuilder, AcBinaryHubProtocolOptions, Action<...>? = null)` that accepts pre-resolved options explicitly. Canonical consumer pattern: +```csharp +var protocolOpts = sp.GetRequiredService>().Value; +hubBuilder.AddAcBinaryProtocol(protocolOpts); +``` + +## Dispatch + +### DISPATCH-1: First-call null response (observed) + +**Status:** Open — not diagnosed +**Affects:** `PostDataAsync` awaiter / OnReceiveMessage → pending-request correlation + +Observed symptom: first `GetProductDtos_80`-style call returns null despite server serializing and sending a valid ~80KB chunked response. Second call (client-side retry) works normally. + +Log timeline: +- Server: `Serialize end (chunked) dataBytes=80571 chunkCount=20` +- Client: `Deserialize end (chunked)` — successful +- Client: `OnReceiveMessage ... requestId=5` +- Client: ~410ms later — `Client received null response. ... requestId=5` +- Client: auto-retry (requestId=6) → full 338 items + +Hypothesis (unverified): `PostDataAsync` awaiter's null-mapping path misroutes the parsed result, or `requestId → Task` correlation has a race on the first response of a fresh connection. Client auto-retry hides the user-visible impact. + +**Related TODO:** `SIGNALR_TODO.md#todo-01` + +## Cross-cutting (also tracked in serializer-side docs) + +### XCUT-1: JSON-in-Binary request parameters — cross-ref + +Same tech debt as `../../AyCode.Core/docs/BINARY_ISSUES.md#xcut-1`. Planned replacement: migrate client→server request parameters from JSON-in-Binary envelope to direct Binary serialization. Coordinated change across all consuming projects. diff --git a/AyCode.Services/docs/SIGNALR_TODO.md b/AyCode.Services/docs/SIGNALR_TODO.md new file mode 100644 index 0000000..157c990 --- /dev/null +++ b/AyCode.Services/docs/SIGNALR_TODO.md @@ -0,0 +1,31 @@ +# SignalR — TODO + +## Priority legend +- **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea + +--- + +## TODO-01: Diagnose first-call null in PostDataAsync +**Priority:** P2 · **Type:** Investigation · **Related:** `SIGNALR_ISSUES.md#issue-02` + +Reproduce the `GetProductDtos_80`-style first-call null. Add trace logs to `PostDataAsync` awaiter path and `OnReceiveMessage → pending request` dictionary lookup. Verify `requestId → Task` correlation on the very first chunked response of a fresh connection. + +## TODO-02: Document asymmetric send/receive capability +**Priority:** P1 · **Type:** Docs + +Current behaviour: sender selects `BinaryProtocolMode` independently; receiver detects the wire format from the first byte (`CHUNK_START=200` → chunked path; else non-chunked). This means client and server can run DIFFERENT `ProtocolMode` settings independently — a core feature amplifying interoperability. + +Document in `SIGNALR_BINARY_PROTOCOL.md` as a dedicated "Asymmetric send/receive contract" section. Key selling points: +- WASM client + AsyncSegment server → works (WASM downgrades send, receives chunked happily) +- Third-party client on NuGet can pick any mode → server doesn't care +- Gradual mode rollouts possible (no synchronized deploy) + +## TODO-03: Code-level guard for FlushTimeout < ClientTimeoutInterval +**Priority:** P2 · **Type:** Feature + +`AcBinaryHubProtocolOptions.Validate()` currently documents (in XML doc) that `FlushTimeout` should be less than the SignalR `HubOptions.ClientTimeoutInterval`, but there is no code-level check. Add validation at protocol registration time — if both options are resolvable from the DI scope, verify the constraint and emit a startup warning (or throw, pending decision). + +## TODO-04: `BinaryProtocolMode.Auto` — adaptive send-mode +**Priority:** P3 · **Type:** Feature · **Related:** `SIGNALR_BINARY_PROTOCOL_TODO.md#todo-03` + +Design: on first received message, inspect first byte to determine peer's send format. On subsequent sends, match it (subject to local-platform constraints, e.g. WASM never actually sends AsyncSegment). Per-`HubConnection` state. Optional upfront handshake-extension negotiation as an alternative — see wire-level TODO. diff --git a/docs/CONVENTIONS.md b/docs/CONVENTIONS.md index a3d424f..7efc916 100644 --- a/docs/CONVENTIONS.md +++ b/docs/CONVENTIONS.md @@ -33,12 +33,7 @@ See `AyCode.Services/docs/SIGNALR.md` for full architecture documentation. ### ⚠️ Temporary: JSON-in-Binary Request Parameters -Client→server request parameters currently use a JSON→Binary→JSON round-trip: -1. `SignalPostJsonDataMessage` serializes `PostData` to JSON string (`PostDataJson`) -2. The JSON-containing message is wrapped in a Binary envelope -3. Server deserializes Binary → extracts JSON string → parses JSON per parameter - -This is **planned for replacement** with direct Binary parameter serialization (matching how responses already work). Do not attempt to fix this as a side-effect of other work — it requires coordinated client+server+consuming-project changes. +Client→server request parameters currently use a JSON-inside-Binary envelope — a cross-cutting tech debt planned for migration to pure Binary. Details in `AyCode.Core/docs/BINARY_ISSUES.md#xcut-1` + `AyCode.Services/docs/SIGNALR_ISSUES.md#xcut-1`. Migration is tracked in `BINARY_TODO.md#todo-01`. Do NOT attempt as a side-effect of unrelated work — requires coordinated client+server+consuming-project changes. ## Testing