From 0ad9250e4cab73192bd26b451517b22872f84fc5 Mon Sep 17 00:00:00 2001 From: Loretta Date: Sun, 26 Apr 2026 13:44:12 +0200 Subject: [PATCH] [LOADED_DOCS: 3 files, no new loads] Add Rule #6, AUTH topic, ADRs, config & doc updates - Codified Rule #6 (authority, rule scope, skill invocation) in all primary copilot-instructions.md files - Clarified skill pre-load/lazy-load rules and LOADED_DOCS prefix - Forbid skill/template version labels in Decision Log governance - Scaffolded new AUTH topic with README, ISSUES, and TODO files - Added repo/project ADR folders and templates; new ADR for AcBinaryHubProtocol decorator stack - Migrated cross-cutting issues/TODOs to Closed with detailed resolution - Made FruitBankHybrid.Shared/appsettings.json the canonical config source; suppressed Razor SDK auto-publish to avoid file collisions - Updated protocol/wire format docs for AcBinaryHubProtocol - Minor config: updated ports, WaitForFlush, and csproj content rules --- .github/LLM_PROTOCOL_DECISIONS.md | 5 + .github/copilot-instructions.md | 9 + .github/skills/adr-author/SKILL.md | 4 +- .../docs-check/references/TOPIC_CODES.md | 1 + AyCode.Core.sln | 10 + AyCode.Core/docs/BINARY/BINARY_ISSUES.md | 4 +- AyCode.Core/docs/BINARY/BINARY_TODO.md | 9 +- AyCode.Core/docs/XCUT/XCUT_ISSUES.md | 41 ++-- .../docs/SIGNALR/SIGNALR_ISSUES.md | 4 +- AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md | 13 ++ .../docs/SIGNALR_BINARY_PROTOCOL/README.md | 4 +- AyCode.Services/docs/adr/0000-TEMPLATE.md | 5 + ...acbinary-decorator-feature-stack-design.md | 215 ++++++++++++++++++ AyCode.Services/docs/adr/README.md | 25 ++ docs/AUTH/AUTH_ISSUES.md | 23 ++ docs/AUTH/AUTH_TODO.md | 22 ++ docs/AUTH/README.md | 54 +++++ docs/adr/0001-user-bearer-token-flow.md | 3 +- docs/adr/README.md | 2 +- 19 files changed, 422 insertions(+), 31 deletions(-) create mode 100644 AyCode.Services/docs/adr/0000-TEMPLATE.md create mode 100644 AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md create mode 100644 AyCode.Services/docs/adr/README.md create mode 100644 docs/AUTH/AUTH_ISSUES.md create mode 100644 docs/AUTH/AUTH_TODO.md create mode 100644 docs/AUTH/README.md diff --git a/.github/LLM_PROTOCOL_DECISIONS.md b/.github/LLM_PROTOCOL_DECISIONS.md index 06d3f67..0b83390 100644 --- a/.github/LLM_PROTOCOL_DECISIONS.md +++ b/.github/LLM_PROTOCOL_DECISIONS.md @@ -58,6 +58,7 @@ Active as of **2026-04-24**. For the full evolution history, see the dated table - **Archive annually** once the log reaches ~50 entries (split to `LLM_PROTOCOL_DECISIONS_.md`). IDs stay unique across archives. - **`Affected` column** uses the scope codes defined below. - **User consent required** for every new entry. +- **No skill / template version labels.** Skills and templates are continuously iterated; the Decision Log entry IS the version history. Reference changes by date or `LLMP-DEC-N` ID, not by synthetic version numbers (`v1.1`, `v2.0`, etc.) — neither in skill content, frontmatter metadata, nor in conversation when discussing changes. LLMP-DEC-45 removed legacy `metadata: version` fields from all SKILL.md files; LLMP-DEC-47 codifies the principle explicitly to prevent the linguistic regression of using `vN.M` as colloquial shorthand for "recent changes". --- @@ -153,6 +154,10 @@ The "primary" vs "inherit" distinction is defined in `protocol-audit/references/ | LLMP-DEC-45 | 2026-04-25 | `adr-author` SKILL.md + ADR_TEMPLATE first-real-use feedback batch (9 changes); `version` field removed from all 5 SKILL.md frontmatters | First end-to-end use of `adr-author` (bearer token ADR session, `0001-user-bearer-token-flow.md`) surfaced 9 friction points across the skill and template, plus consolidates 2 previously-parked feedback items (Copilot session out-of-scope routing, Claude Code session dimensional alternatives — now subsumed). **Changes** (priority order): (#1) Step 8 #3 split into "decision-closes-entry" vs "pre-flight-closed-entry" cases — pre-flight case (entry already closed before ADR formalizes the architecture) gets a soft cross-ref to `### Related` instead of Status update; (#2) new Step 9 "Hand-off after write" — short, ask-don't-assume-next-action; (#3) Step 8 #1 explicit `/README.md` Index table update (not just generic "PRODUCT_DECISIONS.md"); (#4) Step 7 NNNN re-Glob immediately before `Write` (collision safety); (#5) Step 5 opinion-when-asked nuance — frame as preference not prescription, with anti-example pair (❌ "X, egyértelmű." vs ✅ "I lean toward X because Y, but you decide."); (#6) ADR_TEMPLATE Status placeholder fixed `Proposed` → `Proposed (YYYY-MM-DD)` (was inconsistent with skill Step 5); (#7) `docs/adr/0000-TEMPLATE.md` becomes 1-paragraph pointer to canonical `references/ADR_TEMPLATE.md` — eliminates duplication / drift risk; (#8) ADR_TEMPLATE Alternatives section adds optional Reversibility/Cost/Future-flex sub-bullets with explicit "use sparingly" guidance; (#9) ADR_TEMPLATE Status comment notes Status vocabulary distinct from `_ISSUES.md` / `_TODO.md` (Open/InProgress/Closed) — see `TOPIC_CODES.md`. Plus Edge cases "Parallel LLM sessions" updated to point back to Step 7's explicit re-Glob rule (was redundant). **Housekeeping**: `metadata: version` field removed from all 5 SKILL.md frontmatters (`adr-author`, `docs-archive`, `docs-discovery`, `docs-check`, `protocol-audit`). Internal iteration doesn't need versioning ceremony — Decision Log entries are the version history. **No backward-compat concerns**: bearer token ADR (0001) was a test run, not a production consumer; no external dependents on prior skill behaviour. | `adr-author/SKILL.md` (Steps 5/7/8/9, Edge cases, frontmatter); `adr-author/references/ADR_TEMPLATE.md` (Status section, Alternatives section); `docs/adr/0000-TEMPLATE.md` (full rewrite as pointer); `docs-discovery/SKILL.md`, `docs-check/SKILL.md`, `protocol-audit/SKILL.md`, `docs-archive/SKILL.md` (frontmatter `version` field removed) | | LLMP-DEC-46 | 2026-04-25 | `docs-discovery` SKILL.md Step 2: forward-slash mandate added (Windows backslash trap) | **Recurring failure mode**: a Copilot session in VS attempted `file_search` with a Windows-backslash glob pattern (`...\docs\**\*.md`); the glob engine silently returned 0 matches despite files existing, and the agent fell through to code-search per the same false-empty pattern that LLMP-DEC-39 addressed. Same root cause class as the `**/` lehagyás (LLMP-DEC-37, LLMP-DEC-39) — a syntactically malformed pattern returns no match, the agent interprets "no files" as "no docs exist". **Fix**: added a new `### ⚠️ Path separators — forward slashes ONLY in glob patterns` sub-section to `docs-discovery/SKILL.md` Step 2 (placed between the existing `**/` CRITICAL section and "File layout convention"). Content: forward-slash mandate regardless of host OS, correct vs wrong example, failure-mode description (silent 0 matches), absolute rule statement, cross-reference to the `**/` mandate. **Why both rules together**: the `**/` recursive mandate + the `/` separator mandate are the two most common syntactic glob errors observed in real sessions; both produce identical "0 matches → false-empty → code-search fall-through" failure mode. Documenting them adjacent in Step 2 lets the LLM see them as a paired class of pitfalls. **Bug class is host-agnostic**: glob engines uniformly require forward slashes (Microsoft.Extensions.FileSystemGlobbing, ripgrep, Node glob, Python pathlib, JS minimatch) — backslash-in-pattern is silently malformed everywhere. | `docs-discovery/SKILL.md` (Step 2 — new "Path separators" sub-section between the existing `**/` CRITICAL and "File layout convention") | +| LLMP-DEC-47 | 2026-04-25 | Decision Log governance: explicit "No skill / template version labels" clause added | **Recurring slip detected**: even after LLMP-DEC-45 removed `metadata: version` from all 5 SKILL.md frontmatters, the LLM (this agent) continued using "v1.1" colloquially in conversation as shorthand for "the recent batch of changes" — including in a substantive ADR-plan critique. The user pointed out the regression: the slip itself is evidence the principle was not explicit enough. Implicit removal of metadata fields didn't prevent the linguistic ceremony in dialogue or in cross-doc references. **Fix**: added a new bullet to the `Decision Log governance` section in this file's `## Current protocol state (quick reference)` block. The rule covers three surfaces: (a) skill content, (b) frontmatter metadata, (c) conversational / cross-doc references. Reference changes by **date or `LLMP-DEC-N` ID** instead. **Why explicit codification matters**: structural removal (deleting metadata fields) is a one-time mechanical change; linguistic habit is a recurring behavioral one. Implicit principles drift in language even when followed structurally. Codifying prevents the regression at the source — every future agent reading the governance bullets will see the rule before being tempted to introduce `vN.M` shorthand. | `AyCode.Core/.github/LLM_PROTOCOL_DECISIONS.md` (Decision Log governance section — new bullet) | +| LLMP-DEC-48 | 2026-04-25 | `adr-author` SKILL.md Step 1 restructured: fresh-folder creation rule factored out as shared rule (covers fresh-repo + multi-project cases); template wording "copy of" → "1-paragraph pointer to" (sync with LLMP-DEC-45 #7) | **Drift detected by parallel session**: LLMP-DEC-45 #7 made `/docs/adr/0000-TEMPLATE.md` a 1-line pointer to the canonical `references/ADR_TEMPLATE.md` (eliminating duplication / drift risk). The `adr-author/SKILL.md` Step 1 fresh-repo case still said *"Propose creating `0000-TEMPLATE.md` (copy of `references/ADR_TEMPLATE.md`)"* — stale wording. Additionally, the Step 1 multi-project case did not mention 0000-TEMPLATE creation at all (implicit only via inheritance from the fresh-repo case). **Discovery context**: a parallel Claude Code session, planning a project-scoped ADR folder for `AyCode.Services/docs/adr/`, followed the SKILL Step 1 literally and proposed creating a full template copy — which would have created an inconsistent state across the workspace (one folder with pointer, another with copy). **Fix**: SKILL Step 1 restructured. The fresh-folder creation directive (`README.md` + `0000-TEMPLATE.md`) factored out of the fresh-repo case into a **shared rule applying to any newly-created `docs/adr/` folder** (whether at repo root or project-scoped). The 0000-TEMPLATE wording changed from "copy of" to "1-paragraph pointer to" the canonical, with rationale stated in-line ("single source of truth, avoids template drift"). The README.md description also extended to mention the index table. **Universality preserved (no drót)**: SKILL.md does NOT reference LLMP-DEC IDs (workspace-specific) — it states the principle directly so the skill remains portable to any workspace adopting the protocol-stack. The LLMP-DEC link lives only in this Decision Log entry. | `adr-author/SKILL.md` (Step 1 path resolution section — fresh-folder creation factored out as shared rule + canonical-pointer wording) | +| LLMP-DEC-49 | 2026-04-25 | Created `AUTH` topic — registered topic code in `TOPIC_CODES.md`, scaffolded `docs/AUTH/{README, AUTH_ISSUES, AUTH_TODO}.md` at repo root | Bearer-token ADR-0001 (`docs/adr/0001-user-bearer-token-flow.md`, `Accepted (2026-04-25)`) needs a topic home for ongoing issues, planned work, and consumer recipe. **Pre-implementation scaffold**: `README.md` is honest about pre-impl state and includes a Scope table mirroring the ADR's Decision section + a "Security: Never log secrets" guideline (auth-specific phrasing, will trim to cross-ref when `LOG-T-12` lands the canonical version in `LOGGING/README.md`); `AUTH_ISSUES.md` and `AUTH_TODO.md` are empty-state with entry-format templates. **Placement**: repo-root `docs/AUTH/` (not project-level) chosen because AUTH is cross-cutting — affects `AyCode.Services` + `AyCode.Services.Server` + `AyCode.Interfaces` + every consumer. Same parallel-to-projects positioning as `docs/adr/` and `docs/XCUT/` (sibling cross-cutting topics). **`LOG-I-9` / `LOG-I-10`** (auth security leaks closed pre-flight) remain in `LOGGING_ISSUES.md` per their existing Discovery-context note; relocation to `AUTH_ISSUES.md` as `AUTH-I-N` is a separate user-approved follow-up listed in ADR-0001's "Status migration on AUTH topic creation" section — not part of this entry. | `docs-check/references/TOPIC_CODES.md` (new `AUTH` row inserted between `LOG` and `SIG`); 3 new files: `AyCode.Core/docs/AUTH/README.md`, `AUTH_ISSUES.md`, `AUTH_TODO.md` | +| LLMP-DEC-50 | 2026-04-26 | New Rule #6 added to all primary `copilot-instructions.md` — Authority, Rule scope, Skill invocation (3 general behavioral principles) | **Diagnosis from real-world session evidence**: a parallel session reviewing FruitBank plugin docs surfaced 5 distinct misinterpretation patterns: (1) inventing topic codes ad-hoc without consulting the canonical registry; (2) proposing TOC-with-offset re-read patterns that work around the no-re-read rule's scope; (3) inventing custom URL schemes (`@Repo:`) outside markdown standard; (4) proposing empty `_ISSUES.md` files without applying the established Status convention; (5) manually reimplementing a skill's workflow instead of invoking the skill. Five distinct surfaces, **one root cause**: the session knew skills/rules existed at the surface but did not follow references to depth, did not treat authoritative registries as binding, and improvised over canonicalized procedures. **Plus self-evidence**: this assistant slipped THREE times using `vN.M` version-label colloquialism in conversation despite LLMP-DEC-47 explicitly forbidding the pattern — proof that Decision Log entries alone do NOT propagate to behavior. Decision Log governance is not session-start mandatory load; rules in `copilot-instructions.md` are. **Fix**: codify three **general** behavioral principles as Rule #6 in all 5 primary `copilot-instructions.md` files. (a) **Authority before proposal** — verify against authoritative source before inventing categories/IDs/conventions. (b) **Rule implications are part of the rule** — apply rules conservatively, don't work around scope with novel patterns. (c) **Skill invocation preferred over reimplementation** — invoke skills explicitly rather than ad-hoc reimplementation. **General, not workspace-specific**: the rule text mentions NO specific registry, skill, convention, or workspace artifact — it states principles applicable to any registry, any skill, any future workspace. **Why Rule #6, not Rule #2 extension**: Rules #1-5 are state/input/output mechanics (LOADED_DOCS prefix, docs-before-code, no-re-read, context-recovery, modification consent); Rule #6 is **behavioral correctness** of how proposals are formed — separate functional category, separate rule. **Coverage**: Authority covers misinterpretations 1, 3, 4; Rule implications covers 2; Skill invocation covers 5 — full 5/5 of observed patterns. **Why this might still not be enough**: even codified rules slip in agent behavior (the v-label evidence). Multi-LLM independent validation remains structurally necessary as the final safety net; rule-text shifts the probability, doesn't reach 100%. | `5× primary copilot-instructions.md` (new Rule #6 inserted after Rule #5, before `## Session Setup`) | ## Known follow-ups diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 2117f7f..37ed0e5 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -58,6 +58,15 @@ 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. +6. **AUTHORITY, RULE SCOPE, AND SKILL INVOCATION:** + When proposing structural changes (new files, conventions, categories, IDs, naming patterns, URL schemes, etc.), follow these three principles: + + - **Authority before proposal:** Before proposing new structures, conventions, categories, or IDs, verify the proposal against the **authoritative source** for that domain. If a registry, convention doc, or canonical reference exists, follow it (or its registered maintenance workflow for new entries). Do not invent ad-hoc when an authority exists. + + - **Rule implications are part of the rule:** Rules have **scope implications** beyond their literal text. Apply rules **conservatively** — if a rule prohibits action X, do not work around it with novel patterns Y that achieve the same effect. Working around rule scope with ad-hoc patterns is a process violation, not a creative solution. + + - **Skill invocation preferred over reimplementation:** If a workspace skill's scope covers the current task, **invoke the skill explicitly** rather than manually performing similar steps. Skills canonicalize subtleties (validation rules, cross-references, edge cases) that ad-hoc reimplementation risks missing. Improvising a skill's logic is a process violation. + ## Session Setup **Mandatory reads at session start** — in addition to this `copilot-instructions.md`, the agent MUST load the **two reactive workspace skills' `SKILL.md` files** (the three user-gated skills are lazy-loaded on demand — see notes below): diff --git a/.github/skills/adr-author/SKILL.md b/.github/skills/adr-author/SKILL.md index b1c5f4f..2148f39 100644 --- a/.github/skills/adr-author/SKILL.md +++ b/.github/skills/adr-author/SKILL.md @@ -64,13 +64,15 @@ If ambiguous, ask the user explicitly before proceeding. Placement IS a real decision only when no `docs/adr/` exists yet in the relevant scope. Two sub-cases: -- **Fresh repo / first ADR ever**: default to `/docs/adr/` at the repo root. Propose creating it with `README.md` (2-3 lines explaining the convention) and `0000-TEMPLATE.md` (copy of `references/ADR_TEMPLATE.md`). User confirms (Rule #5) before creation. +- **Fresh repo / first ADR ever**: default to `/docs/adr/` at the repo root. - **Multi-project repo, no `docs/adr/` anywhere yet**: match scope to placement — - **Cross-cutting decision** (affects 2+ projects, framework↔consumer boundary, or repo-wide concerns such as auth, logging conventions, build infrastructure) → place at the **highest common ancestor** of affected scopes, typically `/docs/adr/` at the repo root. - **Project-scoped decision** (one project's internal architecture only) → `//docs/adr/`. - **Ambiguous scope** → ask the user explicitly. Suggested phrasing: *"this decision touches projects X and Y — should the ADR live at the repo-root `docs/adr/` (cross-cutting) or scoped to one of those projects?"* +**For any newly-created `docs/adr/` folder** (whether at repo root or project-scoped): propose creating `README.md` (2-3 lines explaining the ADR convention + an index table for new ADRs) and `0000-TEMPLATE.md` (a 1-paragraph pointer to the canonical `references/ADR_TEMPLATE.md` — single source of truth, avoids template drift). User confirms (Rule #5) before creation. + The chosen location persists in git history and inbound cross-references (other ADRs, code comments, docs); relocating later requires updating every cross-ref. Getting placement right at creation is cheaper than fixing it. ## Step 2 — Establish context (Socratic) diff --git a/.github/skills/docs-check/references/TOPIC_CODES.md b/.github/skills/docs-check/references/TOPIC_CODES.md index 7497389..79ae0b0 100644 --- a/.github/skills/docs-check/references/TOPIC_CODES.md +++ b/.github/skills/docs-check/references/TOPIC_CODES.md @@ -13,6 +13,7 @@ To make IDs like `LOG-I-5`, `SIG-B-2`, `XCUT-I-1` **globally unique strings** | Code | Topic | Scope | Docs location (primary) | |---------|-----------------------------|-----------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------| | `LOG` | LOGGING | Logger system: levels, writers, config-reading vs DI factory | `AyCode.Core/AyCode.Core/docs/LOGGING/` (+ variants in `AyCode.Core.Server`, `AyCode.Services`, `Mango.Nop.Services`) | +| `AUTH` | AUTH | User authentication: bearer tokens, JWT, login flow, hub authorization | `AyCode.Core/docs/AUTH/` | | `SIG` | SIGNALR | SignalR transport: tags, client base, dispatch, session | `AyCode.Core/AyCode.Services/docs/SIGNALR/` (+ variant in `AyCode.Services.Server`) | | `SBP` | SIGNALR_BINARY_PROTOCOL | Binary wire protocol over SignalR: framing, chunking, argument read | `AyCode.Core/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/` | | `BIN` | BINARY | AcBinary serializer: features, format, writers, source generator | `AyCode.Core/AyCode.Core/docs/BINARY/` | diff --git a/AyCode.Core.sln b/AyCode.Core.sln index 670e63f..a31383b 100644 --- a/AyCode.Core.sln +++ b/AyCode.Core.sln @@ -64,6 +64,13 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "docs", "docs", "{D4B2E9F1-A docs\GLOSSARY.md = docs\GLOSSARY.md EndProjectSection EndProject +Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "adr", "adr", "{8D1B72D8-7145-4AAB-A216-1D4CC950E3AE}" + ProjectSection(SolutionItems) = preProject + docs\adr\0000-TEMPLATE.md = docs\adr\0000-TEMPLATE.md + docs\adr\0001-user-bearer-token-flow.md = docs\adr\0001-user-bearer-token-flow.md + docs\adr\README.md = docs\adr\README.md + EndProjectSection +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -466,6 +473,9 @@ Global GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE EndGlobalSection + GlobalSection(NestedProjects) = preSolution + {8D1B72D8-7145-4AAB-A216-1D4CC950E3AE} = {D4B2E9F1-A6C3-4F7E-8D5B-3E2A1C4F6B8D} + EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {E97347E0-52CA-4086-A3F4-CE65ABDE9964} EndGlobalSection diff --git a/AyCode.Core/docs/BINARY/BINARY_ISSUES.md b/AyCode.Core/docs/BINARY/BINARY_ISSUES.md index 1f28098..753e208 100644 --- a/AyCode.Core/docs/BINARY/BINARY_ISSUES.md +++ b/AyCode.Core/docs/BINARY/BINARY_ISSUES.md @@ -147,4 +147,6 @@ A single instance must not use context + standalone modes simultaneously — buf ### XCUT-I-1: JSON-in-Binary request parameters — cross-ref -Canonical entry: **`../XCUT/XCUT_ISSUES.md#xcut-i-1`**. Summary: client→server request parameters currently use JSON inside a Binary envelope (`SignalPostJsonDataMessage`); response path is already pure Binary. Planned migration is tracked in `BINARY_TODO.md#bin-t-1` but requires coordinated client+server+consumer changes. Do NOT attempt as a side-effect. +**Status:** Closed (2026-04-26, see canonical entry). + +Canonical entry: **`../XCUT/XCUT_ISSUES.md#xcut-i-1`**. Summary: client→server request parameters previously used JSON inside a Binary envelope (`SignalPostJsonDataMessage`); response path was already pure Binary. Migration landed in commits `cdd54d3` + `3b70070` via `SignalParams` (length-prefixed binary pack/unpack) — wire is now Binary in both directions. Migration plan tracked in `BINARY_TODO.md#bin-t-1` (also Closed). diff --git a/AyCode.Core/docs/BINARY/BINARY_TODO.md b/AyCode.Core/docs/BINARY/BINARY_TODO.md index 6e517a7..387b00f 100644 --- a/AyCode.Core/docs/BINARY/BINARY_TODO.md +++ b/AyCode.Core/docs/BINARY/BINARY_TODO.md @@ -6,12 +6,19 @@ --- ## BIN-T-1: Replace JSON-in-Binary request parameters -**Priority:** P1 · **Type:** Refactor · **Related:** `../XCUT/XCUT_ISSUES.md#xcut-i-1` (canonical), `AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md` +**Priority:** P1 · **Type:** Refactor · **Status:** Closed (2026-04-26, landed in commits `cdd54d3` 2026-04-05 + `3b70070` 2026-04-06) · **Related:** `../XCUT/XCUT_ISSUES.md#xcut-i-1` (canonical), `AyCode.Services/docs/SIGNALR/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. +### Resolution +- **What:** Length-prefixed, per-parameter binary format introduced via `SignalRSerializationHelper.SerializeParametersToBinary` / `DeserializeParametersFromBinary`; further unified into `SignalParams` (single `byte[]` carrying packed method parameters with `SetParameterValues` / `GetParameterValues`). +- **Where:** `AyCode.Services/SignalRs/AcSignalRClientBase.cs`, `AcWebSignalRHubBase.cs`, `ISignalParams.cs` (server + client dispatch); `IAcSignalRHubClient.cs` (legacy wrappers). +- **Equivalent (not literal `SignalPostBinaryDataMessage`):** `SignalParams` was chosen over a 1:1 binary wrapper class — fewer indirections on the hot path, type-safe pack/unpack, and `DataSerializerType` field on `SignalReceiveParams` for response format indication. +- **Wire impact:** No JSON round-trip on the wire for request params; this is a **breaking change** vs. previous JSON-in-Binary clients/servers (see commit message). +- **Legacy types:** `SignalPostJsonMessage`, `SignalPostJsonDataMessage`, `SignalPostMessage`, `ISignalPostMessage` all marked `[Obsolete]` in `IAcSignalRHubClient.cs`; deletion tracked separately in `AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md#sig-t-6` (gated on consumer migration). + ## BIN-T-2: Re-evaluate DiscountProductMapping SGen exclusion **Priority:** P3 · **Type:** Investigation · **Related:** `BINARY_ISSUES.md#bin-i-11` diff --git a/AyCode.Core/docs/XCUT/XCUT_ISSUES.md b/AyCode.Core/docs/XCUT/XCUT_ISSUES.md index 223ca7c..790a65f 100644 --- a/AyCode.Core/docs/XCUT/XCUT_ISSUES.md +++ b/AyCode.Core/docs/XCUT/XCUT_ISSUES.md @@ -8,38 +8,33 @@ For planned cross-cutting work, see `XCUT_TODO.md`. ## XCUT-I-1: JSON-in-Binary request parameters -**Status:** Open +**Status:** Closed (2026-04-26, landed in commits `cdd54d3` 2026-04-05 + `3b70070` 2026-04-06) **Affects:** BINARY serializer (wire format) ↔ SIGNALR transport (envelope) ↔ all consuming projects (caller code) -### Description +### Description (historical) -Client→server request parameters currently travel as JSON inside a Binary envelope: -- `SignalPostJsonDataMessage` is the current envelope -- Response path already uses pure Binary (no JSON) -- Asymmetry: request is JSON-in-Binary, response is Binary +Client→server request parameters previously travelled as JSON inside a Binary envelope: +- `SignalPostJsonDataMessage` was the original envelope +- Response path already used pure Binary (no JSON) +- Asymmetry: request was JSON-in-Binary, response was Binary -### Why it spans multiple topics +### Why it spanned multiple topics -- **BINARY side**: the serializer has to special-case JSON payloads inside a Binary stream; ideal Binary-only flow is broken on the request side. -- **SIGNALR side**: the transport carries the JSON-wrapped Binary payload; `SignalPostJsonDataMessage` is a SignalR-level concept. -- **Consumer side**: every caller sends requests via this asymmetric path; changing the wire format requires coordinated client + server + all-consuming-project updates. +- **BINARY side**: the serializer had to special-case JSON payloads inside a Binary stream; ideal Binary-only flow was broken on the request side. +- **SIGNALR side**: the transport carried the JSON-wrapped Binary payload; `SignalPostJsonDataMessage` was a SignalR-level concept. +- **Consumer side**: every caller sent requests via this asymmetric path; changing the wire format required coordinated client + server + all-consuming-project updates. -### Planned migration (tracked in TODO) +### Resolution -- `BINARY_TODO.md#bin-t-1` — "Replace JSON-in-Binary request parameters" -- Acceptance: `SignalPostJsonDataMessage` replaced by `SignalPostBinaryDataMessage` (or equivalent); no JSON round-trip on the wire for request params; benchmarks confirm no regression. - -### Do NOT attempt as a side-effect - -Any client or server change in isolation will break the other side. Requires: -1. Binary envelope for both request and response defined -2. Client code updated to serialize via Binary -3. Server code updated to deserialize via Binary -4. All consuming projects rebuilt against new API -5. Version bump coordinated +- **What:** Length-prefixed, per-parameter binary format introduced via `SignalRSerializationHelper.SerializeParametersToBinary` / `DeserializeParametersFromBinary`, then unified into `SignalParams` carrying packed parameters as a single `byte[]` with `SetParameterValues` / `GetParameterValues` for type-safe pack/unpack. +- **Where:** `AyCode.Services/SignalRs/` — `AcSignalRClientBase.cs`, `AcWebSignalRHubBase.cs`, `ISignalParams.cs`; legacy types in `IAcSignalRHubClient.cs` marked `[Obsolete]`. +- **Wire impact:** No JSON round-trip on the wire for request params. **Breaking change** vs. previous JSON-in-Binary clients/servers — version bump required. +- **Equivalent, not literal:** `SignalParams` (object[]-style packed payload) replaced `SignalPostJsonDataMessage` instead of introducing a parallel `SignalPostBinaryDataMessage`. Single unified envelope, fewer hot-path indirections. +- **Coordinated change:** client + server + consumer projects updated together in the two commits (BREAKING). All five steps from the original "Do NOT attempt as a side-effect" list completed. +- **Legacy cleanup outstanding (not blocking closure):** `[Obsolete]` wrappers can be deleted once external consumer projects stop referencing them — tracked in `../../../AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md#sig-t-6` (separate from this canonical issue). ### Cross-references - **BINARY_ISSUES.md** (`../BINARY/BINARY_ISSUES.md#xcut-i-1`): cross-ref pointing here - **SIGNALR_ISSUES.md** (`../../../AyCode.Services/docs/SIGNALR/SIGNALR_ISSUES.md#xcut-i-1`): cross-ref pointing here -- **BINARY_TODO.md#bin-t-1**: migration plan +- **BINARY_TODO.md#bin-t-1**: migration plan (also marked Closed) diff --git a/AyCode.Services/docs/SIGNALR/SIGNALR_ISSUES.md b/AyCode.Services/docs/SIGNALR/SIGNALR_ISSUES.md index 3cf9a77..ee9d1f2 100644 --- a/AyCode.Services/docs/SIGNALR/SIGNALR_ISSUES.md +++ b/AyCode.Services/docs/SIGNALR/SIGNALR_ISSUES.md @@ -130,4 +130,6 @@ Hypothesis (unverified): `PostDataAsync` awaiter's null-mapping path misroute ### XCUT-I-1: JSON-in-Binary request parameters — cross-ref -Canonical entry: **`../../../AyCode.Core/docs/XCUT/XCUT_ISSUES.md#xcut-i-1`**. Summary: SignalR transport carries `SignalPostJsonDataMessage` (JSON inside a Binary envelope) for request params, while response path is pure Binary. Planned replacement is coordinated across BINARY serializer + SIGNALR transport + all consuming projects. +**Status:** Closed (2026-04-26, see canonical entry). + +Canonical entry: **`../../../AyCode.Core/docs/XCUT/XCUT_ISSUES.md#xcut-i-1`**. Summary: SignalR transport previously carried `SignalPostJsonDataMessage` (JSON inside a Binary envelope) for request params, while response path was pure Binary. Replacement landed in commits `cdd54d3` + `3b70070`: `SignalParams` (length-prefixed binary pack/unpack) replaces the JSON envelope, coordinated across BINARY serializer + SIGNALR transport + consumer projects. **Breaking change** for older clients. diff --git a/AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md b/AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md index 3484d35..ccf4026 100644 --- a/AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md +++ b/AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md @@ -73,3 +73,16 @@ services.AddSignalR(hubOptions => { /* unchanged */ }) ### Why this belongs in AyCode.Services (framework layer) docs The gap is consumer-level, but the canonical "server-side registration recipe" is a FRAMEWORK responsibility — LOGGING.md already shows it for the logger side. Adding a matching recipe to `../SIGNALR_BINARY_PROTOCOL/README.md#Registration in Program.cs → Server` would prevent the next consumer from making the same mistake. That doc update is part of this TODO's acceptance criteria. + +## SIG-T-6: Delete legacy `[Obsolete]` JSON-in-Binary wrappers +**Priority:** P3 · **Type:** Cleanup · **Related:** `../../../AyCode.Core/docs/XCUT/XCUT_ISSUES.md#xcut-i-1` (closed canonical), `../../../AyCode.Core/docs/BINARY/BINARY_TODO.md#bin-t-1` (closed) + +Now that XCUT-I-1 / BIN-T-1 have landed and the wire format no longer uses JSON-in-Binary, the `[Obsolete]` wrapper types in `IAcSignalRHubClient.cs:60-111` can be deleted: +- `SignalPostJsonMessage` +- `SignalPostJsonDataMessage` +- `SignalPostMessage` +- `ISignalPostMessage` + +**Gate:** verify external consumer projects no longer reference these types — compile-time scan across the workspace + dependent solutions before removal. + +**Acceptance:** all four obsolete types deleted from `IAcSignalRHubClient.cs`; no CS0618 warnings in any consumer build; remaining doc/glossary mentions either deleted or rewritten as historical references. diff --git a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md index 16210df..855435a 100644 --- a/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md +++ b/AyCode.Services/docs/SIGNALR_BINARY_PROTOCOL/README.md @@ -43,7 +43,7 @@ WriteMessage(HubMessage, IBufferWriter output) │ ├─ WriteStringUtf8(invocationId, target) │ ├─ WriteVarUInt(argCount) │ ├─ Per argument: -│ │ ├─ byte[] (AcBinary) → raw length + bytes (no tag, no VarUInt) +│ │ ├─ byte[] (AcBinary) → raw length + bytes (no 0x44 wrapper — first byte is AcBinary FormatVersion 0x01, no VarUInt) │ │ ├─ byte[] (other) → tag (0x44) + raw bytes (no VarUInt — argLength implies size) │ │ └─ object → FlushAndReset() → reserve INT32 arg prefix │ │ → AcBinarySerializer.Serialize(value, output) → patch prefix @@ -63,7 +63,7 @@ var isAcBinary = byteArray.Length >= 2 && (byteArray[1] & 0xF0) == BinaryTypeCode.HeaderFlagsBase; // 0x90 ``` -- **isAcBinary = true**: `[argLength=N][raw AcBinary bytes]` — no tag, reader deserializes directly +- **isAcBinary = true**: `[argLength=N][raw AcBinary bytes]` — no 0x44 wrapper tag; first byte is the AcBinary FormatVersion 0x01 (the discriminator the receiver uses), reader deserializes directly - **isAcBinary = false**: `[argLength=1+N][0x44 tag][raw bytes]` — tag for type detection, no VarUInt (argLength implies size) ### Dual BWO Pattern diff --git a/AyCode.Services/docs/adr/0000-TEMPLATE.md b/AyCode.Services/docs/adr/0000-TEMPLATE.md new file mode 100644 index 0000000..bb99a4c --- /dev/null +++ b/AyCode.Services/docs/adr/0000-TEMPLATE.md @@ -0,0 +1,5 @@ +# ADR Template + +The canonical template lives at [`AyCode.Core/.github/skills/adr-author/references/ADR_TEMPLATE.md`](../../../.github/skills/adr-author/references/ADR_TEMPLATE.md) — that's what the `adr-author` skill copies when authoring a new ADR. + +This file exists only as a placeholder so `docs/adr/` numbering starts visibly at `0001`. Don't edit this file; edit the canonical template if the format needs to change. diff --git a/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md b/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md new file mode 100644 index 0000000..577674a --- /dev/null +++ b/AyCode.Services/docs/adr/0001-acbinary-decorator-feature-stack-design.md @@ -0,0 +1,215 @@ +# ADR 0001: AcBinaryHubProtocol optional feature stack — decorator-based composition design + +## Status + +Proposed (2026-04-25) + +## Context + +`AcBinaryHubProtocol` (`AyCode.Services/SignalRs/AcBinaryHubProtocol.cs`) is the AyCode binary `IHubProtocol` implementation for SignalR — the canonical wire format for binary hub messaging. The base implementation handles AcBinary serialization, three protocol modes (`Bytes` / `Segment` / `AsyncSegment`), chunked AsyncSegment streaming, and the base layer of features every consumer needs. + +A class of optional features is planned for NuGet competitiveness (currently P3 idea status, see `../SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md` SBP-T-5..8): **encryption**, **compression with MinSize**, **OpenTelemetry tracing**, **HMAC signing-only**. These features: + +- Are NOT current-priority — speculative additions for public NuGet positioning. +- Must be **opt-in** (default-off, zero overhead when not used). +- Must be **stackable** in some combinations (encrypt + sign + compress simultaneously). +- Must work with **all three** `BinaryProtocolMode` send strategies (Bytes / Segment / AsyncSegment). +- Each carries its own design + threat model decisions (own ADR per leaf feature). + +This ADR establishes the **composition mechanism** — how these features attach to and stack with `AcBinaryHubProtocol`. Each leaf feature gets a follow-up ADR (0002 encryption, 0003 compression, 0004 tracing, 0005 signing) inheriting this composition decision. Without an explicit umbrella, every leaf ADR would re-derive the same composition pattern → 4× redundancy and risk of drift. + +## Decision + +Adopt the **decorator-chain pattern** for composing optional features onto `AcBinaryHubProtocol`. Each feature is a self-contained `IHubProtocol` implementation that wraps an inner protocol; stacking order is encoded at DI registration time via composition. + +### `AcHubProtocolDecoratorBase` abstract base + +A new abstract class in `AyCode.Services/SignalRs/` (concrete API spec is impl-level, tracked as `SBP-T-9` in `SIGNALR_BINARY_PROTOCOL_TODO.md`): + +- Delegates passthrough `IHubProtocol` members (`Name`, `Version`, `TransferFormat`, `IsVersionSupported`, `GetMessageBytes`) to the wrapped inner protocol. +- Exposes `protected IHubProtocol Inner { get; }` to derived classes. +- Declares virtual `WriteMessage` and `TryParseMessage` with default passthrough impl. +- **Each concrete decorator overrides ONLY the methods relevant to its feature** (~10-15 lines of feature-specific code per decorator; no boilerplate per-impl). + +### Stacking order (canonical) + +`compress → encrypt → sign` (innermost decorator wraps outwards): + +``` +AcBinaryHubProtocol ← CompressingHubProtocol ← EncryptingHubProtocol ← SigningHubProtocol + ↑ + (outermost wrapper) +``` + +DI registration: + +```csharp +services.AddSingleton(sp => + new SignDecorator( + new EncryptDecorator( + new CompressDecorator( + new AyCodeBinaryHubProtocol(opts))))); +``` + +Send order: `AyCodeBinaryHubProtocol` serializes → `CompressingHubProtocol` (innermost wrap) compresses → `EncryptingHubProtocol` encrypts → `SigningHubProtocol` (outermost) appends HMAC tag. Receive order is reversed. + +**Rationale**: compressed ciphertext is uncompressible (compression must precede encryption to avoid wasted CPU AND CRIME/BREACH-class side-channel attacks — TLS 1.3 deprecated TLS-level compression for this reason); encrypt-then-MAC is the industry-standard signing+encryption combination order. The DI registration order encodes this canonically; non-canonical orderings emit a startup warning (validation in DI extension methods — follow-up #2). + +### Handshake-based feature negotiation + +At connection setup (1 application-level handshake roundtrip after SignalR's protocol handshake), both sides exchange a feature manifest: + +``` +{ + "compression": { "active": bool, "algorithm": "lz4" | "brotli" | "zstd" | ... }, + "encryption": { "active": bool, "algorithm": "aes-gcm" | "chacha20-poly1305", "keyId": "..." }, + "signing": { "active": bool, "algorithm": "hmac-sha256" | "hmac-sha512", "keyId": "..." }, + "tracing": { "active": bool, "samplingRate": float? } +} +``` + +`MinSize` for compression is **sender-side config**, NOT a handshake parameter — the two endpoints may use different `MinSize` thresholds. The receiver detects per-message via the `IsCompressed` wire flag (see below). + +**Mismatch handling**: asymmetric registration (one side has decorator, other doesn't) → handshake fails fast with a logged reason at `Error` level. Per-message asymmetry (handshake-skip artefacts) is handled by the decorator-failure unified protocol below. + +The app-level handshake mechanism's concrete shape (Option A: a special `__ac_handshake` HubMessage as first message; Option B: wire-level `WriteHandshake` virtual on `AcHubProtocolDecoratorBase`) is **deferred to a follow-up** — the umbrella commits to "handshake-based" but does not pick the mechanism. Tracked as part of `SBP-T-9`. + +### Per-message `IsCompressed` flag — semantic (A): standalone byte + +The `IsCompressed` flag is a **standalone byte** in the wire format, NOT a value-range extension of the existing message-type byte. Wire format with the compression decorator active: + +``` +[INT32 LE length: 4 byte][IsCompressed: 1 byte][message-type byte: 1 byte][message body: N byte] +``` + +- The `INT32 length` is the existing `AcBinaryHubProtocol` length prefix (unchanged). +- The `length` value INCLUDES the `IsCompressed` byte: `length = 1 (IsCompressed) + 1 (message-type) + N (body) = N + 2`. +- The `IsCompressed` byte takes values `0x00` (uncompressed) or `0x01` (compressed). Future extension values reserved (`0x02..0xFF` unused for now). +- **Per-message overhead: +1 byte / message constant**, regardless of compressed-or-not state. Even uncompressed messages carry the `0x00` byte. + +In `AsyncSegment` mode the `IsCompressed` byte sits at the SAME position (offset 4 in the wire — directly after the INT32 length prefix), BEFORE the `200 CHUNK_START` marker. **Per-chunk overhead: 0 bytes** — the chunk-level framing (`[201][UINT16 size][data]`) is unchanged. + +**Why semantic (A) and not semantic (B) — value-range extension**: an alternative considered was encoding the compressed flag into the unused high bit of the message-type byte (compressed values disjoint from `{1..9, 200..202}`), giving 0 byte overhead in the uncompressed case. Rejected because: +- Adds branching complexity to the receiver (parse high bit, strip it, then dispatch on remaining bits). +- Future feature flags would compete for the same byte's bit space. +- 1 byte / message overhead is negligible at typical message sizes. +- Simpler is better for a NuGet-public protocol. + +### Simple compression rule + +``` +IF handshake.compression.active AND message_size >= MinSize: + → IsCompressed = 0x01, compress whole message bytes (no per-arg inspection) +ELSE: + → IsCompressed = 0x00, passthrough +``` + +No per-arg inspection (no `isAcBinary` magic-byte gating, no mixed-message policy handling). If a developer sends raw byte[] (e.g., already-compressed JPEG) with handshake compression on, the bytes are compressed once more — at most ~1-5% size growth (negligible vs CPU cost; HTTP `Content-Encoding: gzip` follows the same "compress everything" pattern industry-wide). + +The `isAcBinary` detection in the existing wire format remains a per-arg discriminator (first-byte 0x44 vs 0x01) for the byte[] fast-path's serialization branching — **independent** of compression decision. + +**Layer responsibility**: the protocol decorator orchestrates (handshake state, MinSize check, IsCompressed flag emit/read); the `AcBinarySerializer` / `AcBinaryDeserializer` provide the algorithm code (`Compress(bytes, algo)` / `Decompress(bytes, algo)` static helpers). The protocol delegates to these helpers without owning the algorithm implementations. + +### BufferSize semantics — wire-chunk target with internal source-chunk derivation + +`AcBinaryHubProtocolOptions.BufferSize` semantically denotes the **wire-chunk target size** (post-decoration), aligned to the SignalR / Kestrel slab convention (default 4096). The protocol transparently derives the **actual source-chunk size** by subtracting the cumulative per-chunk decoration overhead: + +``` +ActualSourceChunkSize = BufferSize - Σ(decorator.PerChunkOverhead) +``` + +Each decorator MUST expose a `PerChunkOverhead` constant (or `MaxPerChunkOverhead` worst-case bound for compression where output size is data-dependent). The decorator chain sums these at composition time. The chunking logic in `AcBinaryHubProtocol` reads `ActualSourceChunkSize` for source-chunk granularity. + +| Feature | Algorithm | Per-chunk overhead | Type | +|---------|-----------|---------------------|------| +| Encryption | AES-GCM | +28 byte | Fixed (12 nonce + 16 auth tag) | +| Encryption | ChaCha20-Poly1305 | +28 byte | Fixed (12 nonce + 16 auth tag) | +| Signing | HMAC-SHA256 | +32 byte | Fixed (256-bit tag) | +| Signing | HMAC-SHA512 | +64 byte | Fixed (512-bit tag) | +| Compression | LZ4 frame @ 4 KB input | up to +32 byte (worst case) | Bounded | +| Compression | Zstd @ 4 KB input | up to +28 byte (worst case) | Bounded | +| Compression | Brotli @ 4 KB input | up to +160 byte (worst case) | Bounded | +| Tracing | (any) | 0 byte | Fixed | + +**Result**: user-facing `BufferSize` stays Kestrel-slab-aligned regardless of decoration stack. Wire-chunk size ≤ `BufferSize` is guaranteed; `UINT16` chunk size prefix never overflows because BufferSize is well below 65535. + +**Constraint**: `BufferSize` MUST be ≥ 256 byte (sanity floor); decoration stacks summing to ≥ `BufferSize` overhead are rejected at registration time. + +### AsyncSegment per-chunk decoration constraint + +In `AsyncSegment` mode, decorators MUST operate per-chunk (each chunk independently encrypted / compressed / signed) to preserve the protocol's pipeline-parallelism guarantee. Per-message decoration in AsyncSegment defeats the purpose of chunked streaming (the receiver would have to accumulate the entire message before decoding could begin). + +`Bytes` and `Segment` modes have no such constraint and may use per-message decoration freely. + +### UINT16 size prefix design intent (chunk-level) + +The `[201][UINT16 size][data]` chunk framing intentionally caps wire-chunks at 65535 bytes. Larger chunks would defeat the AsyncSegment design: pipeline parallelism benefits diminish past Kestrel slab size (~4 KB), MTU-aligned (~1.5 KB) sizes are typical. A 4-byte INT32 size prefix per chunk would add 2× the overhead at multi-megabyte payloads (20+ chunks × 2 bytes saved per chunk vs the 4-byte alternative). UINT16 is the correct choice for the AsyncSegment use-case. This umbrella ADR codifies the design intent so leaf ADRs do not propose changing it as a side-effect. + +### Decorator-failure unified protocol + +When a decorator's `TryParseMessage` cannot complete (decryption auth-tag fails, decompression error, signature mismatch, opaque feature data on a peer with no matching decorator): + +1. The decorator's `TryParseMessage` returns `false` (per `IHubProtocol` contract — message not parseable). +2. The decorator logs the failure reason at `Error` level via the `ILogger<>` infrastructure (logger threaded through `AcBinaryHubProtocolOptions.Logger`). +3. The SignalR `HubConnection` closes the connection on persistent parse-failure (per SignalR's standard behavior — never silent). + +**Never silent**: a decorator MUST NOT swallow a parse error and return a partial / default message. If the decorator cannot extract the original message, the connection closes with a logged reason. This contract holds for ALL decorators (uniform failure semantics across the entire feature stack). + +### Backward compatibility (handshake-mismatch handling) + +A v1 client (no decorators registered) connecting to a v2 server (decorators in chain), or vice versa, must NOT silently break. The handshake is the SOLE point of feature negotiation: + +- **Handshake mismatch** (one side advertises feature, other does not): handshake fails fast. The connection closes with a logged `Error`-level reason indicating the missing capability. Clients can retry with a reduced feature set (or accept the failure and signal a configuration error to ops). +- **Symmetric DI registration encouraged**: for clean rolling upgrades, both ends of a connection should register matching decorators before either side enables a feature. +- **No on-wire fallback heuristic** — the design relies on handshake state, not first-byte-detection or magic-byte sniffing on a per-message basis. This avoids the fragile heuristic disambiguation that earlier protocol designs (e.g., HTTP/1 vs HTTP/2 on the same port) had to engineer around. + +## Consequences + +### Positive + +- **Single Responsibility**: each feature lives in 1 class, 1 file, 1 concern. `AcBinaryHubProtocol.cs` (~1400 lines) stays unchanged. +- **Truly zero overhead when opted out**: a decorator absent from the DI chain means the feature's code path **does not exist** in the runtime — not JIT-eliminated, not branch-not-taken, but literally no method to invoke. Contrast in-protocol embedding (Alt-2) where opt-out depends on JIT successfully eliminating null-checked branches. +- **Boilerplate mitigated by `AcHubProtocolDecoratorBase`**: passthrough members live in the base; each concrete decorator only writes ~10-15 lines of actual feature code. +- **Composable**: stacking order = DI registration order, runtime-explicit and reviewable. +- **Extensible**: new feature = new `MyFeatureDecorator : AcHubProtocolDecoratorBase` class + DI extension method. Neither the base class nor existing decorators change. +- **Industry-standard handshake negotiation**: matches TLS, HTTP/2 ALPN, WebSocket per-message-deflate, gRPC patterns. Familiar to .NET developers; checks the "polished NuGet" box for procurement / security audits. +- **Backward-compatibility paths preserved**: v1 ↔ v2 mixed deployments fail fast at handshake (early, debuggable) instead of mysteriously corrupting messages. +- **Wire protocol stays minimal**: the only NuGet-feature-related new wire byte is the per-message `IsCompressed` (1 byte). Encryption and signing add only their cryptographic data overhead (nonce + tag); tracing has zero wire impact. + +### Negative + +- **1 vtable indirection per protocol-method call per layer**: 4-deep chain = up to 4 virtual call frames before reaching the inner `AcBinaryHubProtocol`. Negligible at message granularity (sub-microsecond), but real. +- **Debug stack up to 4 frames deeper when all features active**: navigating call stacks in production debugging is mildly noisier. +- **Stacking order fixed at registration**: no runtime-adaptive composition (e.g., "compress only on payloads > 1MB"); intra-decorator logic (like the `MinSize` policy in ADR-0003) handles per-message branching internally. +- **+1 byte / message constant overhead** when compression decorator is in the chain (semantic A: the IsCompressed standalone byte) — accepted as the cost of architectural simplicity. +- **App-level handshake mechanism is new infrastructure**: requires designing an upfront message-exchange before normal traffic. Adds 1 roundtrip to connection setup. Concrete shape (Option A vs B) deferred to a follow-up. + +### Follow-ups required + +1. **`AcHubProtocolDecoratorBase` implementation** — abstract class added to `AyCode.Services/SignalRs/`; tracked as new TODO entry `SBP-T-9` in `SIGNALR_BINARY_PROTOCOL_TODO.md`. Implementation deferred until at least one leaf ADR (0002-0005) reaches `Status: Accepted`. +2. **Stacking-order validation in DI** — `AddAcEncryption` / `AddAcSigning` / `AddAcCompression` extension methods should warn or throw if registered in a non-canonical order (concrete impl part of `SBP-T-9`). +3. **App-level handshake mechanism design** — Option A (`__ac_handshake` HubMessage as first message) vs Option B (`WriteHandshake` virtual on `AcHubProtocolDecoratorBase`). Tracked as part of `SBP-T-9`. +4. **Benchmark scaffolding** — message-granularity vtable cost measurement for the "negligible" claim. Each leaf ADR's acceptance criteria includes verifying this in their feature's context. +5. **Cross-references** — Step 8 of the `adr-author` skill: this ADR links to the 4 forthcoming leaf ADRs (0002-0005); `../SIGNALR_BINARY_PROTOCOL/README.md` gets a new `## Related ADRs` section pointing back to `0001-acbinary-decorator-feature-stack-design.md`. + +## Alternatives considered + +- **In-protocol embedding** (rejected): all features inline inside `AcBinaryHubProtocol` via null-checked `_options` branches. Loses SRP (`AcBinaryHubProtocol.cs` would grow from ~1400 to 2300+ lines), zero-overhead depends on JIT successfully eliminating null branches (not a static guarantee), and cross-feature ordering becomes hardcoded in method bodies. + - *Reversibility:* low — switching to decorator later means a near-total rewrite. + - *Future flexibility:* low — every new feature pressures the monolithic class. + +- **Middleware pipeline** (rejected): ASP.NET Core-style `IAcHubProtocolMiddleware` with `next` delegate chaining. Heavier infrastructure (per-message delegate allocation + closure capture), doesn't align with `IHubProtocol.WriteMessage`'s sync-void contract, and the middleware abstraction shines for HTTP request branching — not linear feature stacking. Effectively reinvents the decorator pattern with extra ceremony. + +- **Source-gen specialization** (rejected): Roslyn generator emits compile-time-specialized `AcBinaryHubProtocol_AesGcm_Lz4_Tracing` types per feature combination. True zero overhead but combinatorial type explosion (4 features × on/off + algorithm choice → 16+ generated types), build-time complexity, and runtime configuration via appsettings becomes architecturally hard. Reconsider only if benchmarks reveal vtable indirection becoming non-negligible at extreme message rates. + - *Cost:* very high. + - *Future flexibility:* low — every feature change is generator surgery. + +- **Per-message wire-marker byte ranges with magic-byte detection** (rejected — earlier draft iteration): each feature would emit a wire-marker byte in `0x10..0xFF` ranges, with the receiver dispatching via first-byte detection. Ruled out because (a) handshake-based negotiation is the industry-standard pattern and avoids fragile per-message heuristics; (b) per-feature wire-marker byte ranges add up to 4 bytes / message overhead in the 4-feature stack, vs the chosen 1 byte for `IsCompressed` plus handshake-decided everything else; (c) backward-compat fail-closed via handshake mismatch is cleaner than per-message disambiguation. + +- **`IsCompressed` as message-type-byte value-range extension (semantic B)** (rejected): encode the compressed flag into the unused high bit of the message-type byte, so uncompressed messages carry 0 added byte overhead. Adds branching complexity to receivers; future feature flags would compete for the same byte's bit space; the per-message overhead saving (1 byte) is negligible at typical message sizes. Semantic (A) — standalone byte — chosen for simplicity and forward extensibility. + +## Related + +- Related TODOs: `../SIGNALR_BINARY_PROTOCOL/SIGNALR_BINARY_PROTOCOL_TODO.md` SBP-T-5 (encryption), SBP-T-6 (compression+MinSize), SBP-T-7 (tracing), SBP-T-8 (signing); SBP-T-9 (`AcHubProtocolDecoratorBase` impl + handshake mechanism — to be added in Step 8 cross-reference round). +- Forthcoming ADRs: 0002 (payload encryption), 0003 (message compression with MinSize), 0004 (OpenTelemetry tracing), 0005 (HMAC signing-only) — all will declare `Depends on: ADR-0001`. +- Topic-folder cross-ref: `../SIGNALR_BINARY_PROTOCOL/README.md` to add `## Related ADRs` section in Step 8. diff --git a/AyCode.Services/docs/adr/README.md b/AyCode.Services/docs/adr/README.md new file mode 100644 index 0000000..b79ee4f --- /dev/null +++ b/AyCode.Services/docs/adr/README.md @@ -0,0 +1,25 @@ +# Architecture Decision Records (ADRs) + +Project-scoped Architecture Decision Records for `AyCode.Services` — design decisions specific to this project's internal architecture (binary hub protocol composition, SignalR client design, etc.). Repo-wide cross-cutting decisions live in [`AyCode.Core/docs/adr/`](../../../docs/adr/), not here. + +See [`.github/skills/adr-author/SKILL.md`](../../../.github/skills/adr-author/SKILL.md) for the full authoring procedure (routing rules, Socratic interview, trade-off elicitation, draft → review → write). + +## Convention + +- **Format:** Nygard-style ADR; one file per decision. +- **Filename:** `NNNN-.md` — zero-padded 4-digit sequence + kebab-case slug derived from the title (e.g. `0001-acbinary-decorator-feature-stack-design.md`). +- **Numbering:** sequential, append-only. Derive next NNNN from `max(existing) + 1` at write time. +- **Template:** copy [`0000-TEMPLATE.md`](0000-TEMPLATE.md) when starting a new ADR. +- **Status field:** `Proposed (YYYY-MM-DD)` → `Accepted (YYYY-MM-DD)` / `Rejected (YYYY-MM-DD)` / `Superseded by ADR-XXXX (YYYY-MM-DD)`. Update in-place; entry body / ID / Decision text remain immutable. + +## Index + +| ID | Title | Status | +|-----------------------------------------------------------------|------------------------------------------------------------------------------------|-----------------------| +| [0001](0001-acbinary-decorator-feature-stack-design.md) | AcBinaryHubProtocol optional feature stack — decorator-based composition design | Proposed (2026-04-25) | + +## Related + +- Repo-wide cross-cutting decisions: [`AyCode.Core/docs/adr/`](../../../docs/adr/). +- Protocol-meta decisions (rule changes, skill additions, instruction-file structural shifts): [`.github/LLM_PROTOCOL_DECISIONS.md`](../../../.github/LLM_PROTOCOL_DECISIONS.md). +- Topic-level cross-references for `AcBinaryHubProtocol`-specific ADRs surface in [`SIGNALR_BINARY_PROTOCOL/README.md`](../SIGNALR_BINARY_PROTOCOL/README.md) under `## Related ADRs` (added in Step 8 of the `adr-author` skill — separate `mehet` round). diff --git a/docs/AUTH/AUTH_ISSUES.md b/docs/AUTH/AUTH_ISSUES.md new file mode 100644 index 0000000..d389fb3 --- /dev/null +++ b/docs/AUTH/AUTH_ISSUES.md @@ -0,0 +1,23 @@ +# AUTH — Known Issues + +Active known issues for user authentication (bearer tokens, JWT, login flow, hub authorization). + +For planned/actionable work see [`AUTH_TODO.md`](AUTH_TODO.md). +For the architectural decision that scoped this topic, see [`../adr/0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md). + +## Active entries + +*(No `AUTH-I-N` entries yet — topic just created. Currently-relevant security entries live as `LOG-I-9` and `LOG-I-10` in [`LOGGING_ISSUES.md`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md), pre-migration. ADR 0001's "Status migration on AUTH topic creation" follow-up is the planned home transfer — separate user-approved task.)* + +## Entry format + +When adding the first entry, follow the standard `_ISSUES.md` shape used across topics (LOGGING, BINARY, SIGNALR), with `AUTH-I-N` ID format per [`TOPIC_CODES.md`](../../.github/skills/docs-check/references/TOPIC_CODES.md): + +- ID line with `**Severity:** ... · **Status:** ... · **Area:** ...` +- `### Description` — concrete problem with quotable evidence +- `### Root cause` — what's actually wrong (optional) +- `### Fix direction` — proposed approach +- `### Resolution` — when Status moves to `Closed` (per `TOPIC_CODES.md` Status conventions): what / where / why / caveat +- `### Related` — sibling entries, cross-topic refs, ADR refs (`**Reference:** ADR-NNNN` for ADR pre-flight cross-refs per `adr-author/SKILL.md` Step 8 #3) + +Status vocabulary (per `TOPIC_CODES.md`): `Open`, `InProgress`, `Closed (YYYY-MM-DD)`. Three values only — distinct from ADR Status (Proposed/Accepted/Superseded/Rejected). diff --git a/docs/AUTH/AUTH_TODO.md b/docs/AUTH/AUTH_TODO.md new file mode 100644 index 0000000..7f7a000 --- /dev/null +++ b/docs/AUTH/AUTH_TODO.md @@ -0,0 +1,22 @@ +# AUTH — TODO + +Planned work for user authentication (bearer tokens, JWT, login flow, hub authorization). + +For known issues, see [`AUTH_ISSUES.md`](AUTH_ISSUES.md). +For the architectural authority, see [`../adr/0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md) — its Follow-up sections ("Implementation", "Consumer wiring") are the master plan for what should populate this file. + +## Priority legend + +- **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea + +## Active entries + +*(No `AUTH-T-N` entries yet — topic just created. ADR 0001 Follow-up "Implementation" + "Consumer wiring" lists the master plan; granular `AUTH-T-N` entries will populate this file as implementation phases kick off and break the high-level plan into trackable units.)* + +## Entry format + +When adding the first entry, follow the standard `_TODO.md` shape used across topics (LOGGING, SIGNALR, BINARY), with `AUTH-T-N` ID format per [`TOPIC_CODES.md`](../../.github/skills/docs-check/references/TOPIC_CODES.md): + +- ID line with `**Priority:** P1/P2/P3 · **Type:** Feature/Refactor/Bug fix/etc. · **Related:** ` +- Short rationale (1-2 sentences) +- Optional sections: Acceptance criteria, Migration plan, Consequences/checklist diff --git a/docs/AUTH/README.md b/docs/AUTH/README.md new file mode 100644 index 0000000..568e53b --- /dev/null +++ b/docs/AUTH/README.md @@ -0,0 +1,54 @@ +# AUTH — User Authentication + +Bearer-token user authentication: JWT issuance, client-side token storage, HTTP + SignalR transport bindings, per-tag SignalR dispatch authorization, and security hardening. + +> **Architectural authority:** [`../adr/0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md). This README is the consumer-facing recipe; the ADR captures rationale and decision history. + +## Status + +⚠️ **Pre-implementation.** ADR 0001 specifies the architecture; the framework code has not yet landed. This README is a scaffold — the "Consumer recipe" section is a placeholder for content that fills in as the implementation progresses (per ADR 0001 Follow-up "Implementation" series). + +## Scope + +| Concern | Decision | +|---|---| +| Token issuance | JWT (HS256, 24h fixed lifetime, configurable per consumer); refresh-flow deferred to ADR 0002 | +| Client-side storage | `IAcTokenStore` framework abstraction; consumer-supplied platform impls (MAUI / WASM) | +| HTTP transport | Framework-provided delegating handler injects bearer header automatically | +| SignalR transport | Framework hub-builder extension wires access-token provider; WASM uses query-string fallback | +| Server-side validation | Standard ASP.NET Core JwtBearer pipeline via framework DI bundle | +| Per-tag authorization | Hub-class `[Authorize]` default + explicit allowlist attribute for unauth tags (login, ping) | +| Security hardening | Startup options validation (key length, issuer/audience), HTTPS metadata required outside Development | + +## Out of scope (per ADR 0001) + +Refresh-token flow (→ ADR 0002), role/claims authorization, multi-tenant claims, OAuth2 / external IdP, token revocation, logout server-side invalidation. + +## Files in this folder + +- [`README.md`](README.md) — this file (scope + consumer recipe). +- [`AUTH_ISSUES.md`](AUTH_ISSUES.md) — known issues, security observations, edge cases. +- [`AUTH_TODO.md`](AUTH_TODO.md) — planned work derived from ADR 0001 Follow-ups. + +## Consumer recipe + +> Placeholder — populated as implementation progresses. + +Sections to come (mirrors ADR 0001 Follow-up "Implementation" + "Consumer wiring"): +- DI registration bundle examples (MAUI + WASM + Server) +- `IAcTokenStore` consumer implementation skeletons +- Hub allowlist attribute usage +- `appsettings.json` `AyCode:Jwt` section reference +- HTTPS / Kestrel / proxy notes (especially WASM `access_token` URL-param redaction) + +## Security: Never log secrets + +JWT signing keys, access tokens, refresh tokens, password hashes, and OAuth client secrets MUST NEVER appear in log output. Logged secrets leak via the same channels as the logs themselves (file system, retention archives, screenshots, bug reports). For diagnostics, log only metadata (user ID, expiry, issuer) or hash prefixes — never the raw value. + +This guideline emerged from `LOG-I-9` and `LOG-I-10` (both `Closed (2026-04-25)` via `#if DEBUG` gating per ADR 0001 pre-flight). Pending `LOG-T-12` proposes adding a generalized framework-level guideline section to [`LOGGING/README.md`](../../AyCode.Core/docs/LOGGING/README.md); when that lands, it becomes the canonical home and this section trims to a cross-ref. + +## Cross-references + +- **ADR**: [`../adr/0001-user-bearer-token-flow.md`](../adr/0001-user-bearer-token-flow.md) — architectural decision. +- **Topic registry**: `AUTH` row in [`TOPIC_CODES.md`](../../.github/skills/docs-check/references/TOPIC_CODES.md) (added in `LLMP-DEC-49`). +- **Sibling pre-migration**: `LOG-I-9` / `LOG-I-10` in [`LOGGING_ISSUES.md`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md) — currently homed in LOGGING; ADR 0001 Follow-up "Status migration on AUTH topic creation" lists these as candidates for relocation here as `AUTH-I-N` with `Superseded by` cross-refs in LOGGING. Migration NOT yet performed — separate user-approved follow-up. diff --git a/docs/adr/0001-user-bearer-token-flow.md b/docs/adr/0001-user-bearer-token-flow.md index c95b73e..242b11c 100644 --- a/docs/adr/0001-user-bearer-token-flow.md +++ b/docs/adr/0001-user-bearer-token-flow.md @@ -2,7 +2,7 @@ ## Status -Proposed (2026-04-25) +Accepted (2026-04-25) ## Context @@ -99,6 +99,7 @@ Adopt a layered bearer-token authentication architecture across all client and s - **Hub-class-only `[Authorize]` without per-tag allowlist** (rejected): login + ping tags would also require auth → bootstrap deadlock. - **Per-tag DI policy registry** (rejected): too much complexity for Layer 0; mixes consumer-specific tag lists into framework. - **Custom token middleware from scratch** (rejected): non-standard, no `[Authorize]` interop, no out-of-the-box JWT validation. + - *Future flexibility:* locks us out of standard OAuth2 / external-IdP integration when ADR `0002+` extends scope; the chosen `JwtBearer` pipeline keeps that door open. - *Future flexibility:* locks out standard OAuth2 / external-IdP integration when ADR `0002+` extends scope; the chosen `JwtBearer` pipeline keeps that door open. ## Related diff --git a/docs/adr/README.md b/docs/adr/README.md index 3dd8c51..cba07d5 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -16,7 +16,7 @@ See [`.github/skills/adr-author/SKILL.md`](../../.github/skills/adr-author/SKILL | ID | Title | Status | |-------------------------------------------------|------------------------------------------------------------------------|-----------------------| -| [0001](0001-user-bearer-token-flow.md) | User bearer-token authentication flow (HTTP + SignalR + tag dispatch) | Proposed (2026-04-25) | +| [0001](0001-user-bearer-token-flow.md) | User bearer-token authentication flow (HTTP + SignalR + tag dispatch) | Accepted (2026-04-25) | ## Related