From 8f725937411a007f48ccaa782aaf564e25feebb7 Mon Sep 17 00:00:00 2001 From: Loretta Date: Sat, 25 Apr 2026 20:24:32 +0200 Subject: [PATCH] [LOADED_DOCS: 3 files, no new loads] ADR skill, status vocab, and security log fixes - Enhanced `adr-author` skill and ADR template based on first use: clarified opinion sharing, cross-referencing, index updates, and parallel session handling; rewrote ADR template pointer and added first repo-wide ADR. - Updated skill loading: only `docs-discovery` and `docs-check` pre-loaded; others lazy-loaded; removed `version` from SKILL.md frontmatters. - Simplified issue/TODO status to `Open`, `InProgress`, `Closed`; updated archiving logic and migrated all entries. - Added forward-slash glob pattern rule to `docs-discovery/SKILL.md`. - Closed LOG-I-9 and LOG-I-10: DEBUG-gated sensitive log lines, fixed typo, and cross-referenced new ADR. - Various documentation and index improvements. --- .github/LLM_PROTOCOL_DECISIONS.md | 3 + .github/copilot-instructions.md | 2 +- .github/skills/adr-author/SKILL.md | 30 ++++- .../adr-author/references/ADR_TEMPLATE.md | 18 ++- .github/skills/docs-archive/SKILL.md | 1 - .github/skills/docs-check/SKILL.md | 1 - .github/skills/docs-discovery/SKILL.md | 14 ++- .github/skills/protocol-audit/SKILL.md | 1 - AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md | 28 ++++- .../Logins/AcLoginServiceServer.cs | 2 +- docs/adr/0000-TEMPLATE.md | 5 + docs/adr/0001-user-bearer-token-flow.md | 109 ++++++++++++++++++ docs/adr/README.md | 24 ++++ 13 files changed, 222 insertions(+), 16 deletions(-) create mode 100644 docs/adr/0000-TEMPLATE.md create mode 100644 docs/adr/0001-user-bearer-token-flow.md create mode 100644 docs/adr/README.md diff --git a/.github/LLM_PROTOCOL_DECISIONS.md b/.github/LLM_PROTOCOL_DECISIONS.md index fe5ed9a..06d3f67 100644 --- a/.github/LLM_PROTOCOL_DECISIONS.md +++ b/.github/LLM_PROTOCOL_DECISIONS.md @@ -151,6 +151,9 @@ The "primary" vs "inherit" distinction is defined in `protocol-audit/references/ | LLMP-DEC-43 | 2026-04-25 | Lazy-load shift for user-gated skills + new `docs-archive` skill + Status field convention | **Three integrated changes consolidated into one decision** because they share the underlying principle "user-gated skills are lazy-loaded; reactive skills stay pre-loaded": (A) **Lazy-load shift**: `protocol-audit` and `adr-author` (LLMP-DEC-41) move from Session Setup pre-load to lazy-load. The `## Shared Agent Skills` bullets in `copilot-instructions.md` already contain enough trigger description for the LLM to recognize when to invoke; the full SKILL.md only needs to be in context at execution time. Token savings: ~10-14K per session (3 SKILL.md × ~5K avg). (B) **New `docs-archive` skill** (lazy-loaded, user-gated + LLM-suggest-back): rotates closed entries (`Status` ∈ {Fixed, Resolved, Won't fix, Superseded by X}) from active `_ISSUES.md` / `_TODO.md` / `LLM_PROTOCOL_DECISIONS.md` into year-bucketed `*_.md` archive files. Year of Status update determines destination bucket. Status-based filter (no foundational-flag complexity, no age threshold). Active file gets a 2-line pointer block at top; archive files NOT auto-loaded by `docs-discovery` (lazy-on-suspicion read pattern documented in `docs-discovery/SKILL.md` "Archive files" section). (C) **Status field convention** formalized in `TOPIC_CODES.md`: explicit value list (`Open`, `Partially Fixed`, `Fixed`, `Resolved`, `Won't fix`, `Documented limitation`, `Superseded by`), date stamping rules, partial-fix `### Resolution status` sub-section format, archive-eligibility filter. Status updates are the **one exception** to append-only — field is mutable; entry body / ID / Description remain immutable. **Cross-skill integration**: `docs-discovery/SKILL.md` gets new "Archive files" section (default-exclude year-suffixed glob; on-demand read on regression / supersession-ref / cross-ref signals). `docs-check` already had status-update-on-fix logic (LLMP-DEC-27); this entry formalizes the value vocabulary it should use. **Workflow**: `docs-archive` reports per-file analysis (Step 2), user selects scope (Step 3), generates plan with diff (Step 4), applies on explicit consent (Step 5, Rule #5). Archive operation is on-demand — no automatic monitoring, no background size checks. **Note**: an earlier in-conversation draft proposed the lazy-load shift as a standalone LLMP-DEC entry; folded into this LLMP-DEC-43 instead, since lazy-load and `docs-archive` skill share the same design rationale and benefit from being recorded as one decision. | new: `AyCode.Core/.github/skills/docs-archive/SKILL.md`; updated: `docs-check/references/TOPIC_CODES.md` (new "Status field conventions" section); `docs-discovery/SKILL.md` (new "Archive files" section); `5× primary` `copilot-instructions.md` (Session Setup: 4→2 pre-loaded SKILL.md, prefix count 5→3, lazy-load list added); `3× inherit` `copilot-instructions.md` (Session Setup: 4→2 pre-loaded SKILL.md, prefix count 6→4, lazy-load list added); `4× non-Core primary` + `3× inherit` `copilot-instructions.md` (Shared Agent Skills intro updated; new `docs-archive` bullet added after `adr-author`) | | LLMP-DEC-44 | 2026-04-25 | Status field vocabulary simplified: 7 values → 3 (`Open`, `InProgress`, `Closed`); bulk-migrated all existing TODO/Issue entries to `Open` | **Refinement of LLMP-DEC-43 (C)**: the 7-value Status vocabulary (`Open`, `Partially Fixed`, `Fixed`, `Resolved`, `Won't fix`, `Documented limitation`, `Superseded by`) was deemed over-engineered. **Reduced to 3 values**: `Open` (active/unresolved; also used for documented-current-behaviour entries that must remain visible), `InProgress` (partial work in flight), `Closed` (done — bug fixed, decision made, TODO completed; archive-eligible). Distinctions previously encoded in the Status (Fixed vs Resolved vs Won't fix vs Superseded) move to the **entry body** — body explains "what happened" (date, ref, rationale); Status only signals "is this still in the active set?". Documented-current-behaviour entries (previously `Documented limitation`, `By design`, `Acceptable`, `Limitation`, `Upstream SDK limitation`, etc.) stay as `Open` with an optional body callout (`> **Note:** This entry documents accepted current behaviour — not scheduled for change.`). **Bulk migration** applied: every existing `Status` value in `_ISSUES.md` and `_TODO.md` files across the workspace (LOGGING, XCUT, BINARY, SIGNALR, SBP) was set to `Open` as a clean-slate starting point — future Status updates use the new 3-value vocabulary. **Archive criterion** in `docs-archive` simplified to a single rule: `Status == "Closed"` → archive-eligible. **Known anomaly**: `SIGNALR_ISSUES.md` had a single `Status: DONE` entry — also set to `Open` per the bulk-migration rule, but semantically should likely be `Closed` (work was actually done). User to manually re-mark if appropriate. **Why simpler is better**: the previous 7 values created false precision — the distinction between Fixed/Resolved/Won't fix matters in the body, not in the Status field. The `Documented limitation` value semantically conflicted with archiving (it's not "closed work"); folding it into `Open` with a body callout removes the conflict. Archive logic becomes a single equality check. | `docs-check/references/TOPIC_CODES.md` (Status section: 7 → 3 values, simpler workflow); `docs-archive/SKILL.md` (archive criterion simplified to `Status == "Closed"`); 6 files affected by bulk Status migration: `LOGGING_ISSUES.md`, `XCUT_ISSUES.md`, `BINARY_ISSUES.md`, `SIGNALR_ISSUES.md`, `SIGNALR_BINARY_PROTOCOL_ISSUES.md`, `SIGNALR_BINARY_PROTOCOL_TODO.md` (`TOON_ISSUES.md` unchanged — all real entries already `Open`) | +| 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") | + ## Known follow-ups *(No open follow-ups. All items from previous audits resolved — see dated entries above for fix history.)* diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 1f01be2..2117f7f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -73,7 +73,7 @@ The remaining three skills are **lazy-loaded** — `SKILL.md` is read on demand **Path resolution**: if this repo is the canonical protocol host (see `@repo` block below — typically AyCode.Core), the paths are local: `.github/skills//SKILL.md`. Otherwise, prefix with this repo's `own-dep-repos` AyCode.Core path (see the `## Shared Agent Skills` section below for explicit paths). -**Why mandatory**: workspace skills are NOT in Claude Code's native skill-registry / system-reminder. Without pre-loading their `SKILL.md` content, the agent cannot reliably recognize implicit triggers (e.g., "domain question → invoke docs-discovery" at the moment the question arrives, not retroactively). Pre-loading is a **one-time cost** per session (~10-13K tokens); Rule #3 (no-re-read) prevents repeated reads; re-read only if Rule #4 (Context Recovery) fires. +**Why mandatory**: workspace skills live as plain `SKILL.md` files in `.github/skills/` — LLMs do NOT auto-discover them via any built-in skill-registry. Without pre-loading their `SKILL.md` content, the agent cannot reliably recognize implicit triggers (e.g., "domain question → invoke docs-discovery" at the moment the question arrives, not retroactively). Pre-loading is a **one-time cost** per session (~5-8K tokens for the 2 reactive `SKILL.md` files; the 3 user-gated skills are lazy-loaded — no upfront cost); Rule #3 (no-re-read) prevents repeated reads; re-read only if Rule #4 (Context Recovery) fires. **Amortization — critical, do NOT re-evaluate per-turn**: the Session Setup cost is measured over the ENTIRE session, not per single turn. A typical session has many turns; the first domain question alone already recoups the investment (alternative — repeated source-code `Grep`/`Read` per turn — costs 10-20K tokens *per turn* with lower output quality). Do NOT flag pre-loaded content as "wasteful" for turns that don't invoke it — the design depends on cross-turn amortization + Rule #3 (no-re-read) + on-demand specificity of Rule #4 (Context Recovery). This amortization is the **central token-economics principle** of the entire protocol stack. diff --git a/.github/skills/adr-author/SKILL.md b/.github/skills/adr-author/SKILL.md index 5e46e1c..b1c5f4f 100644 --- a/.github/skills/adr-author/SKILL.md +++ b/.github/skills/adr-author/SKILL.md @@ -4,7 +4,6 @@ description: Create Architecture Decision Records (ADRs) for architecturally sig compatibility: Designed for Claude Code and GitHub Copilot (VS). Uses the host agent's Read/Write/Edit/Glob/Grep tools. metadata: author: Fullepi - version: "1.0" --- # adr-author @@ -105,7 +104,14 @@ Keep it concise: 2-4 bullets per category per alternative. Not a dissertation. ## Step 5 — Decision capture -**The user chooses. You never decide.** Your job is to surface, weigh, and ask clarifying questions. After the user commits: +**The user chooses. You never decide.** Your job is to surface, weigh, and ask clarifying questions. + +**When the user explicitly asks for your opinion** ("mi a véleményed?", "what do you think?", "which would you pick?"): share your weighing — but frame it as preference, not prescription. End with the user's decision still open. + +- ❌ Avoid: *"X is obviously the right choice."* / *"Path: repo-root, egyértelmű."* +- ✅ Prefer: *"I lean toward X because Y, but you decide."* + +After the user commits: - Confident → `Status: Accepted (YYYY-MM-DD)` - Tentative/experimental → `Status: Proposed (YYYY-MM-DD)` — revisit later @@ -130,7 +136,7 @@ Assemble the draft using `references/ADR_TEMPLATE.md`. Show the draft to the use ### Per-repo ADR write - Path: `/NNNN-.md` — `` is the location resolved in Step 1 (single-project repos default to `/docs/adr/`; multi-project repos follow the placement rules in Step 1's "Multi-project repo case"). -- NNNN: zero-padded 4 digits (e.g., `0001`, `0042`). Derive from `Glob /*.md` and take max+1. +- NNNN: zero-padded 4 digits (e.g., `0001`, `0042`). **Re-Glob `/*.md` immediately before the `Write` call** (not at draft time) — parallel sessions or interim work may have added entries; deriving NNNN at draft time risks collision. - ``: short kebab-case derived from the title (e.g., `cursor-based-pagination`, `postgres-over-mongo`). ### LLMP decision log write @@ -143,11 +149,23 @@ Assemble the draft using `references/ADR_TEMPLATE.md`. Show the draft to the use After the primary write: -1. **Quick-reference update** — if the repo has a `PRODUCT_DECISIONS.md` (or equivalent ADR index), append a new row. +1. **Quick-reference / index update** — append a new row to whichever ADR-index file exists in the repo. Most commonly: `/README.md` (which should have an Index table). Other patterns: `PRODUCT_DECISIONS.md` at the repo root, or none. If no index exists, propose creating one in `/README.md` as part of the same write. 2. **Supersession chain** — if this ADR supersedes an earlier one, edit the older ADR's `Status` line: `Superseded by ADR-NNNN (YYYY-MM-DD)`. -3. **Issue/TODO resolution** — if this decision closes an open entry in `{TOPIC}_ISSUES.md` or `{TOPIC}_TODO.md`, propose updating those entries with `Status: RESOLVED by ADR-NNNN`. Do not apply without user consent. +3. **Issue/TODO resolution** — two cases: + - **Decision-closes-entry**: if this ADR's decision resolves an `Open` entry in `{TOPIC}_ISSUES.md` or `{TOPIC}_TODO.md`, propose updating those entries to `Status: Closed (YYYY-MM-DD)` with a `### Resolution` sub-section referencing this ADR (per `TOPIC_CODES.md` Status conventions). + - **Pre-flight-closed-entry**: if an entry was closed BEFORE the ADR write (a pre-flight fix that this ADR formalizes architecturally), do NOT change its Status — it's already `Closed`. Instead, propose appending a soft cross-ref to the entry's `### Related` section: `**Reference:** ADR-NNNN — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences.` + + Do not apply either case without user consent. 4. **Follow-up TODOs** — if Step 6 surfaced follow-up work items, propose their addition to the relevant `{TOPIC}_TODO.md` as new `{TOPIC}-T-N` entries. Do not apply without user consent. +## Step 9 — Hand-off after write + +After Steps 7 + 8 complete, do not assume what comes next. Ask the user explicitly: + +> "ADR-NNNN written and cross-references updated. What's next? Common options: commit changes / promote `Status: Proposed → Accepted (YYYY-MM-DD)` / start implementation work / open the next ADR for a related decision. Which?" + +Keep it short — let the user steer. If the user already indicated direction in earlier turns ("after this ADR, we'll start implementation"), reflect that in the hand-off rather than enumerating from scratch. + ## Handoff with other skills - **`docs-check`** — when docs-check (at the end of a code-modifying response) notices an observation that looks ADR-worthy (irreversible decision landed in code; alternatives were actively weighed in the session; cross-cutting impact), it SHOULD flag: *"this may warrant an ADR — consider invoking `adr-author`"*. Does not auto-invoke. @@ -163,7 +181,7 @@ Tool-neutral. Map to the host agent's tools: ## Edge cases -- **Parallel LLM sessions / numbering collision**: derive NNNN at the moment of write, not earlier. If two ADRs end up with the same number, the later writer re-numbers and updates cross-refs. Rare; manual resolution. +- **Parallel LLM sessions / numbering collision**: covered by Step 7's "Re-Glob immediately before Write" rule. If two ADRs nonetheless end up with the same NNNN (concurrent writes within the glob-to-write window), the later writer re-numbers and updates cross-refs. Rare; manual resolution. - **User changes mind mid-interview**: if the user indicates "actually we don't need to decide this yet", exit cleanly. Offer to write a `_TODO.md` entry instead so the question isn't lost. - **Retroactive ADR for a decision already made in code**: legitimate. Document it honestly: `Status: Accepted (retroactive, YYYY-MM-DD), originally decided ~`. The value is the rationale preservation, even after-the-fact. - **Decision spans multiple repos**: write the primary ADR in the repo that owns the change most-centrally; add short `See /docs/adr/NNNN-.md` cross-refs in the other affected repos' ADR folders (under the same NNNN if possible; otherwise a free slot in each). diff --git a/.github/skills/adr-author/references/ADR_TEMPLATE.md b/.github/skills/adr-author/references/ADR_TEMPLATE.md index c109ea8..97253ce 100644 --- a/.github/skills/adr-author/references/ADR_TEMPLATE.md +++ b/.github/skills/adr-author/references/ADR_TEMPLATE.md @@ -11,9 +11,15 @@ ## Status - + + +Proposed (YYYY-MM-DD) ## Context @@ -52,9 +58,17 @@ Proposed - **** (rejected): + - *Reversibility:* + - *Cost:* + - *Future flexibility:* - **** (rejected): ## Related diff --git a/.github/skills/docs-archive/SKILL.md b/.github/skills/docs-archive/SKILL.md index f1a4de3..04af02a 100644 --- a/.github/skills/docs-archive/SKILL.md +++ b/.github/skills/docs-archive/SKILL.md @@ -4,7 +4,6 @@ description: Rotate closed entries (Status: Fixed/Resolved/Won't fix/Superseded compatibility: Designed for Claude Code and GitHub Copilot (VS). Uses the host agent's Read/Write/Edit/Glob/Grep tools. metadata: author: Fullepi - version: "1.0" --- # docs-archive diff --git a/.github/skills/docs-check/SKILL.md b/.github/skills/docs-check/SKILL.md index e24f12c..8439c92 100644 --- a/.github/skills/docs-check/SKILL.md +++ b/.github/skills/docs-check/SKILL.md @@ -4,7 +4,6 @@ description: Evaluate loaded `.md` documentation at the end of every code-modify compatibility: Designed for Claude Code and GitHub Copilot. Invoked at the end of code-modifying responses. Uses only content already in LOADED_DOCS. metadata: author: Fullepi - version: "1.0" --- # docs-check diff --git a/.github/skills/docs-discovery/SKILL.md b/.github/skills/docs-discovery/SKILL.md index afb7c91..3d1c5bd 100644 --- a/.github/skills/docs-discovery/SKILL.md +++ b/.github/skills/docs-discovery/SKILL.md @@ -4,7 +4,6 @@ description: Load all .md documentation relevant to the user's current coding ta 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 @@ -54,6 +53,19 @@ The `**/` is NOT cosmetic. It matches `docs/` at **any depth** in the workspace: **The rule is absolute**: NEVER drop the leading `**/`, even when you "know" the repo. Let the recursive glob find the actual depth. Relative-path guesses based on "usual" layouts are a reliable source of false-empty conclusions. +### ⚠️ Path separators — forward slashes ONLY in glob patterns + +Glob engines (ripgrep, `Microsoft.Extensions.FileSystemGlobbing`, Node `glob`, Python `pathlib`, etc.) **all expect forward slashes `/` in patterns**, regardless of host OS. The filesystem layer handles OS-level path normalization; the pattern stays POSIX. + +**Correct (always)**: `H:/Applications//**/docs/{TOKEN}/**/*.md` +**Wrong (Windows trap)**: `H:\Applications\\**\docs\{TOKEN}\**\*.md` + +**Failure mode**: a Windows-backslash glob typically returns **0 matches even when files exist** — silent failure. The agent sees "no docs", falls through to code-search, and never realizes the search was malformed. + +**Rule is absolute**: write `/` in every glob pattern. If your tool/runtime is Windows and you're tempted to "match the OS path style" — don't. The glob engine will not normalize backslashes for you in pattern position. + +(Combined with the `**/` mandate above: every glob in this skill uses `/**/` with forward slashes throughout — both rules together cover the most common false-empty conclusions.) + ### File layout convention (See `LLM_PROTOCOL_DECISIONS.md` entry "Docs migrated to folder+README pattern".) diff --git a/.github/skills/protocol-audit/SKILL.md b/.github/skills/protocol-audit/SKILL.md index 985069f..9dab05b 100644 --- a/.github/skills/protocol-audit/SKILL.md +++ b/.github/skills/protocol-audit/SKILL.md @@ -4,7 +4,6 @@ description: Audit all `.github/copilot-instructions.md` files registered in `re compatibility: Designed for Claude Code and GitHub Copilot (VS). Requires read access to the paths listed in `references/REPOS.md`. metadata: author: Fullepi - version: "2.2" --- # Protocol Audit diff --git a/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md b/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md index f1b196f..ba535ea 100644 --- a/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md +++ b/AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md @@ -174,7 +174,7 @@ See `LOGGING_TODO.md#log-t-11`. ## LOG-I-9: JWT signing key written to log (CRITICAL security) -**Severity:** 🛑 **Critical (security)** · **Status:** Open · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` +**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` ### Description `GenerateAccessToken` writes the JWT signing key to the logger via `GlobalLogger.Detail`: @@ -194,14 +194,25 @@ Log storage is typically at a lower trust level than dedicated secret stores. Th ### Fix direction Remove the `GlobalLogger.Detail($"Key: ...")` line entirely. The signing key must NEVER appear in any log output. If presence verification is needed for diagnostics, log only a hash prefix (e.g. `Key configured: SHA256[0..8] = abc12345`). +### Resolution + +**What:** Wrapped the `GlobalLogger.Detail($"Key: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block explaining the risk. Line preserved (not deleted) — user opted for DEBUG-gating over removal so local auth-debug retains the diagnostic. + +**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` (line numbers shifted slightly due to the inserted comment). + +**Why:** User-directed deviation from the originally proposed "remove entirely" fix direction (bearer-token ADR planning session, 2026-04-25). DEBUG-gating ensures production (`-c Release`) builds strip the line at compile time, while preserving dev-time diagnostics. + +**Caveat:** DEBUG builds still write the key to log. CI/CD must build with `-c Release` for production deployments. The inline `// SECURITY:` comment in the code warns future maintainers. + ### Related +- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. - `LOG-I-10` (sibling — access token also leaked, same file, same code path) - `LOGGING/README.md` may need a new "Never log secrets" guideline section (separate `_TODO.md` candidate) - **Discovery context**: this issue and `LOG-I-10` emerged during the user-bearer-token ADR context-gathering phase (2026-04-25 Copilot session, while reading `AcLoginServiceServer.cs`). If a dedicated AUTH topic-folder is created later (likely as part of the bearer token ADR follow-ups), these two entries are candidates for relocation to `AUTH_ISSUES.md` with `Status: SUPERSEDED by AUTH-I-N` cross-references. ## LOG-I-10: JWT access token written to log (CRITICAL security) -**Severity:** 🛑 **Critical (security)** · **Status:** Open · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` +**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` ### Description After `GenerateAccessToken` produces a token, the token text itself is written to the logger: @@ -217,7 +228,20 @@ Access tokens are bearer credentials — anyone holding the token can authentica ### Fix direction Remove the `GlobalLogger.Detail($"AccesToken: ...")` line entirely. If issuance verification is needed, log only **metadata** — user ID, expiry timestamp, issuer — never the token itself. +### Resolution + +**What:** Two changes: +1. Wrapped the `GlobalLogger.Detail($"AccessToken: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block. +2. Fixed the `AccesToken` → `AccessToken` typo (flagged in original Description as a side-note) in the log-message string literal. + +**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` (line numbers shifted slightly due to the inserted comment). + +**Why:** Same as `LOG-I-9` — user opted for DEBUG-gating over deletion (2026-04-25, bearer-token ADR session). Typo fix is opportunistic since the line was being touched. + +**Caveat:** Same DEBUG-build leak constraint as `LOG-I-9`. CI/CD must build with `-c Release` for production. + ### Related +- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences. - `LOG-I-9` (sibling — JWT signing key also leaked, same file) - Same `LOGGING/README.md` "Never log secrets" guideline gap as LOG-I-9 - Same discovery-context note as LOG-I-9 (bearer token ADR session, candidate for AUTH topic relocation if/when that emerges) diff --git a/AyCode.Services.Server/Logins/AcLoginServiceServer.cs b/AyCode.Services.Server/Logins/AcLoginServiceServer.cs index 2e03f1e..fda7691 100644 --- a/AyCode.Services.Server/Logins/AcLoginServiceServer.cs +++ b/AyCode.Services.Server/Logins/AcLoginServiceServer.cs @@ -224,7 +224,7 @@ public class AcLoginServiceServer` validator. +- Repo-wide grep follow-up: search for any other call sites that may log secrets in similar patterns (preventive). + +*Consumer wiring (per-consumer projects, separate tasks):* +- MAUI token-store implementation (consumer side). +- WASM token-store implementation (consumer side). +- Per-consumer `appsettings.json` `AyCode:Jwt` section. +- Per-consumer hub: tag allowlist for login + ping (at minimum). + +*Status migration on AUTH topic creation:* +- Move `LOG-I-9` and `LOG-I-10` to `AUTH_ISSUES.md` per their existing "Discovery context" notes; in `LOGGING_ISSUES.md` replace bodies with cross-ref `Superseded by AUTH-I-N`. + +*Future ADRs:* +- `0002` — Refresh-token flow + sliding-vs-fixed lifetime decision. +- Possibly later: role/claims authorization, multi-tenant claims, OAuth2 / external-IdP (per the deferred OUT-of-scope items). + +## Alternatives considered + +- **In-memory-only token store** (rejected): app-restart re-login on MAUI is unacceptable UX. +- **Per-call manual `Authorization` header injection** (rejected): boilerplate + error-prone; framework should encapsulate. +- **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 out standard OAuth2 / external-IdP integration when ADR `0002+` extends scope; the chosen `JwtBearer` pipeline keeps that door open. + +## Related + +- Pre-flight fix entries: [`LOGGING_ISSUES.md#log-i-9`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md), [`LOGGING_ISSUES.md#log-i-10`](../../AyCode.Core/docs/LOGGING/LOGGING_ISSUES.md) (both `Closed (2026-04-25)`). +- Tentative TODO surfaced post-fix: `LOG-T-12` ("Never log secrets" guideline section in `LOGGING/README.md`). +- External: RFC 6750 (Bearer Token Usage), RFC 7519 (JWT), Microsoft.AspNetCore.SignalR auth docs (query-string token pattern). +- Future: ADR `0002` (refresh-flow design). diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 0000000..3dd8c51 --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,24 @@ +# Architecture Decision Records (ADRs) + +Repo-wide Architecture Decision Records — design decisions that affect two or more projects in this repo, framework↔consumer boundaries, or other repo-wide concerns. Project-scoped decisions (internal to a single sub-project) belong in that project's `/docs/adr/` folder, 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-user-bearer-token-flow.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-user-bearer-token-flow.md) | User bearer-token authentication flow (HTTP + SignalR + tag dispatch) | Proposed (2026-04-25) | + +## Related + +- Protocol-meta decisions (rule changes, skill additions, instruction-file structural shifts) live in [`.github/LLM_PROTOCOL_DECISIONS.md`](../../.github/LLM_PROTOCOL_DECISIONS.md), not here. +- Cross-cutting issues / TODOs (without architectural decision component) live in [`docs/XCUT/`](../XCUT/README.md).