merge main into this branch - #18
Merged
moeller-projects merged 15 commits intoSep 26, 2026
Merged
Conversation
…e routing bypass)
Implements implementation-plans/P1-high/P1-13-api-key-auth-case-bypass.md
- ApiKeyEndpointFilter on the /reviews route group replaces middleware path matching
- Deleted ApiKeyAuthenticationMiddleware (ordinal StartsWithSegments gate was
bypassable: POST /REVIEWS routed case-insensitively but skipped auth)
- Endpoints mapped under MapGroup("/reviews") so enforcement cannot diverge
from routing; segment-exact matching preserved (/reviewsx -> 404)
- Tests: ApiKeyAuthTests case-variant paths (/REVIEWS, /ReViews) -> 401,
near-miss paths -> 404, filter unit tests for fail-closed/dev-opt-out branches
Refs: P1-13
Implements implementation-plans/P1-high/P1-11-status-aware-dedupe-regression-reopen.md - ReviewContext gains ResolvedKeys (stage-1 threads with Fixed/Closed status); populated at the top of ExecuteReasoningStage — no extra ADO fetch - ReviewCollector gains MarkRegressed/RegressedKeys; ReviewTools.RecordFinding and the homoglyph path accept a verbatim re-submission of a resolved key as a regression finding (IsRegression), exactly once per regression; Active threads keep the existing MarkRedetected silence - ThreadTriage.Plan emits Reopen + "Regressed in <sha>" note for Fixed/Closed threads whose key regressed; the reopen outranks the pending-reply manual flag - PublishFindingsStage suppresses the duplicate new thread for a reopened regression and re-stamps the reopened thread id for persistence - CommentFormatter marks regressions with a "⚠️ regressed:" heading prefix - Tests: ReviewTools/ThreadTriage/CommentFormatter/ExecuteReasoningStage units, stage-level triage+publish, end-to-end F1 scenario in ServiceTests Refs: P1-11
…stale shells Implements implementation-plans/P1-high/P1-12-shell-aware-backoff-and-reaper.md - FailureBackoff.BlockedUntil ignores runs with CompletedAt=null (shells): a crash between BeginRun and PersistRun no longer reads as a failure streak - ShellReaperService (hosted, one-shot): finalizes stale shells (Success=false, CompletedAt=now) preserving finding rows via the SaveRunAsync upsert, so discovery backoff applies a bounded window from reaper time - Reap extracted as internal ReapOnceAsync: .NET 10 BackgroundService.StartAsync schedules ExecuteAsync via Task.Run(stoppingCts.Token); an immediate StopAsync cancels the token before the delegate runs and StopAsync suppresses the cancellation, silently dropping the reap - DiscoveryService: in-flight claim check ordered before backoff attribution; in-flight PRs report "already in flight", never "head failing", and head-failing-backoff counts only real failures - ReviewWorker PersistFailureAsync uses CancellationToken.None (never the cancelled stoppingToken) with a linked 5s timeout - IFindingStore.GetStaleShellsAsync + SqliteFindingStore query (CompletedAt == null, StartedAt < cutoff), FakeFindingStore support - Tests: DomainTests shell backoff, SqliteFindingStoreTests.GetStaleShells, ShellReaperServiceTests (finalize + no-op), DiscoveryServiceTests in-flight claim precedence, ReviewWorkerTests host-cancel mid-run Refs: P1-12
…d files Implements implementation-plans/P1-high/P1-14-quoted-diff-paths.md - New DiffPathParser: decodes git C-style-quoted path tokens (octal \NNN, escaped \" \\ \t \n \r \a \b \f \v, UTF-8 reassembly); bare fast path with no allocations; malformed input reports false instead of throwing - DiffIndex: diff --git / +++ / Binary files / rename to headers all route through the shared parser, fixing both the quoted-path blind spot (a file like payloäd.cs previously vanished from the diff, homoglyph scan, and scope guard) and the " b/" substring mis-split; a +++ header that fails to decode no longer registers the file, so the manifest match below fails - HomoglyphDiffAnalyzer: same parser; header/hunk ordering mirrors DiffIndex (in-hunk content consumed before +++ headers) so an added line starting with "+++ b/" is scanned as content, never a file switch - PrepareRepositoryStage: a manifest file with no diff entry now throws (fail closed) instead of warn-and-drop; budget-excluded files logged at Debug; binary/mode-only/rename/truncated whitelist unchanged - Tests: DiffPathParser table (quoting rules, malformed headers), DiffIndex quoted-path end-to-end and fail-closed, analyzer ascii-vs-quoted parity + ordering, stage-level quoted/binary/manifest-orphan behavior Refs: P1-14
Implements implementation-plans/P1-high/P1-15-reporeadtools-symlink-deny-bypass.md - Resolve now re-applies the deny list to the symlink-resolved repo-relative path and refuses any read that traverses a symlink inside the checkout: a committed symlink to a contained-but-denied file (.env, appsettings.*.json, *.pem) can no longer launder the path past the deny list into the LLM context - PathSafety.ResolveReal gains an out linksTraversed (component-level link count, relative to the resolved root so environment symlinks above the checkout do not false-positive) - Grep re-denies per file about to be opened (stat'ing reserved for those files) and skips symlink-traversed entries; List hides them; both reuse one IsResolvedDenied helper. Denial messages stay non-oracular (identical "access denied" template, no symlink/escape hint) - Deviation from plan text: helper named ResolveReal(path, out int) rather than TryResolveReal — resolution has no failure mode a Try-pattern can report honestly; Grep deny counts not logged at Debug — RepoReadTools has no logger dependency - Tests: symlink to denied target (Read + Grep), symlink to allowed file (still refused, intentional), deep chain a -> b -> .env, symlinked dir as Grep path, List hiding, non-oracular message equality Refs: P1-15
…ingle-row reads Implements implementation-plans/P2-performance-ops/P2-26-finding-store-retention.md - IFindingStore.PruneAsync: deletes runs older than the retention cutoff in one immediate SQLite transaction (findings first, then runs with RETURNING), always keeping the last MinRunsPerPr runs per PR and never the latest completed run (dedupe continuity) — the carry-forward design previously grew finding rows quadratically in run count per PR - GetLastCompletedRunAsync: single-row id lookup via rowid DESC LIMIT 1 raw SQL (per-PR runs never overlap, so rowid order is completion order) plus one findings load — no longer materializes every completed run - GetRecentRunsAsync: ORDER BY rowid DESC LIMIT in SQL; findings load only for the selected runs; failure-backoff streak semantics unchanged (per-PR sequential runs make rowid order identical to the old StartedAt order) - Runs composite index extended with CompletedAt for the prunable set - RetentionOptions (ReviewForge:Retention:Days=30 / MinRunsPerPr=5), validated fail-fast at startup; DiscoveryService prunes at the sweep tail, throttled to at most once per hour via TimeProvider - Tests: prune keeps min-runs/latest-completed and cascades findings, prune exemption for an old completed run behind newer shells, 50-run latest correctness, 300x40 bounded-read scale, sweep prune call + hourly throttle Refs: P2-26
Implements implementation-plans/P2-performance-ops/P2-27-homoglyph-scan-allocations.md - Options.Default: shared immutable defaults (ImmutableHashSet) resolved once per scan instead of a record + two HashSets allocated per line - Vectorized all-ASCII fast path in ScanLine skips tokenization for lines that can never flag; analyzer skips Preprocess for pure-ASCII added lines - Span-based TokenizeRanges with inline ASCII tracking: substrings materialized only for tokens that flag; script distinctness via a PopCount bitmask (no per-token ToArray/Distinct) - Analyzer iterates diff.AsSpan().EnumerateLines (no Split array + 30k line strings) - NFKC retained per token: ScanLine is public and takes unnormalized input — fullwidth "var" only flags because NFKC maps it, so plan step 3.4's removal would regress detection (deviation, verified by fullwidth parity case) - Tests: parity theory vs verbatim pre-P2-27 reference scanner (16 default-options corpus cases + custom-options cases), Options.Default equivalence, 10k-line GC allocation budget (<2 MB); also fix two pre-existing xUnit analyzer warnings (xUnit2000 arg order, xUnit2031 Where-before-Single) in test files from earlier waves Refs: P2-27
…tion, non-backtracking regex Implements implementation-plans/P2-performance-ops/P2-28-grep-aggregate-budget.md - Grep aborts at an aggregate time (10 s) or line (200k) budget with a "…[truncated: budget-time|budget-lines]" marker; the tool description teaches the model to narrow and retry (string result shape kept — trailer markers per existing "…[match cap reached]" convention) - CancellationToken parameter on Grep, checked per file and per line; MEAI binds it from the agent loop's token (verified by an AIFunctionFactory invoke test) - Patterns compile with RegexOptions.NonBacktracking (linear-time, no catastrophic backtracking); constructs it rejects (lookarounds/backreferences) fall back to the interpreted engine with the 2 s per-line timeout, marked "…[pattern-fallback]" - Enumeration yields FileInfo so the size cap reuses the enumeration stat instead of re-statting; P1-15 deny/containment checks unchanged - Budgets configurable via the new validated RepoReadTools:GrepMaxMs/GrepMaxLines section (RepoReadToolsOptions), threaded through ReviewPipelineFactory -> AgentOptions -> RepoReadTools ctor (positive-only, ctor + DataAnnotations validated) - Tests: line/time budget truncation, mid-scan cancellation, MEAI token binding, lookaround fallback + catastrophic-pattern completion, ctor budget validation Refs: P2-28
…static Implements implementation-plans/P2-performance-ops/P2-31-run-log-buffering.md - RunLogWriter: AutoFlush=false with a 64 KB buffer; a provider-level 2 s timer flushes open writers and CloseRun/Dispose flush+close — agent-loop Debug lines no longer serialize + flush synchronously per entry (default MinLevel is now Information; Debug stays opt-in via RunLogs:MinLevel) - Bounded closed-run id set (cap 10k): post-close scoped writes are dropped instead of re-creating a leaked writer; _Disposed guard drops post-dispose writes - Disposal races are swallowed (ObjectDisposedException/IOException) at the provider and writer, including StreamWriter.Dispose's re-flush — no logging exception can escape into pipeline code; writer/stream seams make the failure paths testable - RunLogFileProvider.Current static deleted: ReviewWorker now depends on the injected IRunLogLifecycle; DI registers the provider itself when enabled, a no-op otherwise - Log dir 0700 / files 0600 on Unix (CodexCredential pattern); P3-x permissions half folded in here - Tests: 1000-entry completeness after CloseRun, post-close drop (replaces the old reopen-append test per the new behavior), concurrent write-vs-dispose smoke, deterministic disposed-writer swallow via the factory seam, closed-set bounding, Unix mode assertions (skipped on Windows), default-level pin, IO-failure flush/ dispose via a throwing stream; worker test sites use NoopRunLogLifecycle Refs: P2-31
Implements implementation-plans/P2-performance-ops/P2-32-checkout-size-refresh.md - RepoCheckoutPool no longer walks the full checkout tree on every AcquireAsync reuse: the reuse fast path drops RefreshCachedSize entirely (~200k syscalls saved per run on a 100k-file checkout); the clone path keeps its one-time materialization measurement - Size cache entries carry a RefreshedAt stamp; the eviction sweep re-measures entries older than one hour (fake-clock test proves fresh stamps are trusted and stale ones re-walked before budget enforcement) - Pool takes an optional TimeProvider so stamps and the sweep clock never mix; a failed re-measure now keeps the stale value instead of pretending the checkout is empty - Telemetry gauge reads CheckoutDirectoryCount only — no behavioral change - Tests: CountingFs proves acquire-materialize walks once and reuse walks zero extra; FakeTimeProvider test proves hour-stale refresh triggers budget eviction Refs: P2-32
Implements implementation-plans/P2-performance-ops/P2-29-prompt-text-sanitization.md - New PromptText.Clean = StripDelimiters(TextSanitizer.Sanitize(x)) — one trust boundary for every PR-author-controlled string in the prompt: title, description, work-item fields, thread replies, changed-file list, and the shrunk diff body; ReviewTools.ReadContext applies it to staged context payloads too - Invisible/bidi/tag-block Unicode (e.g. U+E0049.. "IGNORE") no longer reaches the model from any PR-controlled field, closing the gap where detectors saw the sanitized view but the model saw raw text - Strip-only, no NFKC: legitimate non-Latin source (Cyrillic comments, fullwidth identifiers) passes through byte-faithful for anchoring; detectors keep applying PipelineText.Preprocess themselves - PromptBuilder.Sanitize renamed to StripDelimiters to end the two-"Sanitize" ambiguity; structural enrichment (tool-supplied) keeps delimiter-strip only - DiffExclusions.IsExcluded reuses RepoPath.Normalize instead of duplicating it - Tests: tag-block instruction absent from the built prompt (no TextSanitizer- forbidden char survives anywhere), bidi override stripped with visible text preserved, non-Latin diff content not normalized, full-prompt stable snapshot; P1-5 delimiter-forgery suite stays green Refs: P2-29
Implements implementation-plans/P2-performance-ops/P2-30-homoglyph-whole-token-rule.md - Confusables expanded from Unicode 16.0.0 confusables.txt (Cyrillic/Greek/ Armenian Latin-lookalikes + visual Arabic-Indic digits); existing repo entries kept where confusables.txt's prototype differs - New ScanLine branch: skeleton fully ASCII but different from the token -> 'whole-token lookalike' (medium); keyword skeletons still claim High first - Analyzer maps three rule ids incl. homoglyph/whole-token-lookalike - security.json gains the whole-token rule entry - Parity corpus cases the rule intentionally reclassifies (ΑΤΜ, ｖａｒ, аbc both-scripts, single-char а) asserted explicitly against the new rule - Tests: HomoglyphDetectorTests whole-token cases, analyzer ruleId/severity, rulebook entry; allocation budget unchanged Refs: P2-30
Implements implementation-plans/P2-performance-ops/P2-33-endpoint-submit-backoff-key.md - FailureBackoff.BlockedUntil: a failure record with empty HeadSha is head-agnostic — it counts toward the streak for any requested head and never breaks one; headed mismatches still break (discovery unchanged) - Endpoint submit path is not a backoff consumer (claims-only) — behavior pinned via the discovery sweep integration test - Tests: FailureBackoffTests head-less streak/mixed-history cases, DiscoveryServiceTests sweep skips head whose fetch failures are headless Refs: P2-33
moeller-projects
merged commit Sep 26, 2026
0d2a3c3
into
dependabot/nuget/Microsoft.Agents.AI-1.22.0
4 of 13 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.