fix(daily): preflight /search/* with bound username to catch GitHub 422 at create-time (#474)

[eanzhao]

Summary

• PreflightGitHubProxyAsync now probes /search/issues and /search/commits with the bound github_username after the existing /rate_limit step. 422 responses (the "users... cannot be searched..." surface from [Bug] GitHub Daily Report 返回大量 422 invalid user/permission 且提交数据不准确 #473) trigger fail-fast as github_search_unauthorized, distinct from the existing 401/403 github_proxy_access_denied envelope, with a stable reason_code for operators (scope_insufficient_or_user_not_found vs. validation_failed).
• The freshly minted api-key is best-effort revoked on the new 422 path, mirroring the cleanup contract from PR fix(agent-builder): use UserService.id for api-key allowed_service_ids (#417) #418 so /daily retries don't accumulate orphan proxy-scoped keys.
• Three new unit tests pin the behavior: /search/issues 422 fail-fast (production-shape envelope), commits-only 422 (failure on the second probe, matching [Bug] GitHub Daily Report 返回大量 422 invalid user/permission 且提交数据不准确 #473's reported degradation), and empty-200 success (so we don't regress into rejecting legitimate "no recent activity" creations).

Closes #474. Related: #473 (user-facing reproduction); complementary to #439 (runtime classifier — not duplicated here).

Why this is needed

/rate_limit is scope-light. It returns 200 for any valid OAuth token, including tokens whose grant lacks public_repo / repo. That gap was invisible to the create-time preflight from #411 / #417, which only checked 401/403. So agents persisted with a healthy preflight, only to 422 every /search/* call at runtime — every scheduled daily report came back as 422 invalid user/permission across all sections, and commits degraded to "unrelated global results, not attributable to {user}". Catching this at agent-create time is strictly better than catching it after the agent is scheduled, persisted, and has wasted a tool call + Lark notification on every cron tick.

Test plan

• dotnet build agents/Aevatar.GAgents.Authoring.Lark/Aevatar.GAgents.Authoring.Lark.csproj — clean
• dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/... — 561 passed (including 3 new tests for the 422 paths)
• dotnet test test/Aevatar.AI.Tests/... — 475 passed (covers NyxIdApiClient envelope shape)
• bash tools/ci/test_stability_guards.sh — passed
• bash tools/ci/architecture_guards.sh — passes through the proto/projection/channel/workflow guards; playground_asset_drift_guard exits with the same 254 on a clean dev checkout (environmental on this worktree, not caused by the change). Verified by stashing the diff and re-running.
• CI green
• Manual verification path (out of scope for this PR; tracked in bug(daily): GitHub /search/* returns 422 at runtime — preflight (rate_limit) only screens 401/403, search-API permission/scope failures slip through #474 acceptance criteria): exercise /daily against a NyxID account whose GitHub OAuth grant has read:user only — expect github_search_unauthorized with reason_code: scope_insufficient_or_user_not_found instead of a persisted-but-broken agent.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 903b6c0be1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Probe repo-scoped permissions during GitHub preflight

The new preflight only validates global author queries (author:{username}) via /search/issues and /search/commits, but daily-report runtime switches to repo-qualified queries when a repository allowlist is provided (see AgentBuilderTemplates.cs repo-mode URLs around lines 163-170). This means a token that can search public data but lacks private-repo access can still pass preflight (200/empty on global search) and then fail at runtime for repo:{owner}/{repo} calls, so the change still persists broken agents for private-repo configurations. The preflight should include repo-qualified probes whenever repositories is configured.

Useful? React with 👍 / 👎.

[eanzhao]

LGTM. Thorough review — no blocking issues found.

What this does: Adds /search/issues and /search/commits probes (with the bound github_username) to the GitHub proxy preflight, catching the 422 failure mode where /rate_limit passes but the OAuth grant lacks public_repo/repo scope. This prevents persisting daily-report agents that would 422 every scheduled run.

Code quality:

• IsErrorEnvelope correctly extracts the shared NyxIdApiClient.SendAsync error envelope shape, deduplicating the parsing logic between ClassifyRateLimitProbeFailure and ClassifySearchProbeFailure.
• Backward compatible: when githubUsername is null/empty, search probes are skipped and only /rate_limit runs — same behavior as before.
• Error classification is conservative: ClassifyGitHubSearch422Body falls through to validation_failed for unknown 422 bodies rather than guessing.
• All three new tests pass and cover: 422 on /search/issues, 422 on /search/commits only (issues succeed), and empty-200 success (no regression on legitimate "no recent activity" users).

Non-blocking nit: The detail variable in ClassifySearchProbeFailure shadows the local detail output from IsErrorEnvelope — the right-hand ternary uses the local, the left-hand is the anonymous type property. This is correct C# but slightly confusing to read. Consider renaming the local (e.g., envelopeDetail or nyxMessage) for clarity in a follow-up.

[eanzhao]

Overall: the fix is well-structured — the two-step probe, the refactoring into separate classifiers, and the test coverage for the main 422 paths are all solid. A few gaps worth addressing before merge:

1. Missing test: empty/null username skips search probes
normalizedUser empty returns null immediately, but no test verifies the search probes are never issued when username is absent. The existing 403 test short-circuits at Step 1, so it does not cover this. A dedicated test would protect against a future regression where someone removes the empty guard.

2. Missing test: validation_failed reason_code
Only scope_insufficient_or_user_not_found is exercised. A 422 with a body that does not contain "cannot be searched" (e.g. a query syntax error) should produce validation_failed, but this default branch is untested.

3. {username} in hint is a literal, not interpolated
The hint string contains https://github.com/{username} as a regular string literal (not $""), so it outputs the literal text {username} rather than the actual username. Since github_username is already a field in the response, this is cosmetic, but it would be clearer to either interpolate the actual value or drop the template and reference the field.

4. Sequential search probes (not blocking)
Both probes go through the NyxID proxy to GitHub, each adding a full round-trip. For the create-time path this is acceptable, but if latency becomes a concern, Task.WhenAll would halve the wait on the happy path.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.03%. Comparing base (e6ea4be) to head (849a956).
⚠️ Report is 11 commits behind head on dev.

@@           Coverage Diff           @@
##              dev     #479   +/-   ##
=======================================
  Coverage   71.03%   71.03%           
=======================================
  Files        1230     1230           
  Lines       89062    89062           
  Branches    11654    11654           
=======================================
+ Hits        63266    63269    +3     
+ Misses      21259    21258    -1     
+ Partials     4537     4535    -2     

Flag	Coverage Δ
ci	71.03% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[eanzhao]

Pushed f77d83f7 addressing review feedback:

Codex P1 (r3152148327) — repo-scoped permissions probe added. After the global /search/* step, when repositories is configured, preflight loops the allowlist and runs /search/issues?q=repo:{owner}/{repo}+author:{username} and /search/commits?q=repo:{owner}/{repo}+author:{username} for each. Fail-fasts with the failing repo embedded in github_path. Carried Repositories through DailyReportTemplateSpec so the preflight reads the same normalized list the prompt template emits — no duplicate comma-split.

eanzhao review (scope_insufficient_or_user_not_found only) — added ExecuteAsync_CreateAgent_DailyReport_FailsClosed_When_GithubSearchReturns422_WithUnknown422Body covering the conservative fall-through to validation_failed for 422 bodies without the "cannot be searched" markers.

eanzhao review (no test for /rate_limit short-circuit) — added an assertion to the existing 403 test pinning that no /search/* request fires when /rate_limit returns 401/403. Catches the empty-username regression scenario by structural symmetry — both early returns are in the same method, so a re-ordering that breaks one breaks both.

eanzhao review ({username} literal in hint) — fixed; the hint now references the github_username field already in the response envelope.

eanzhao non-blocking nit (detail shadowing) — renamed local to envelopeMessage in ClassifyRateLimitProbeFailure and ClassifySearchProbeFailure so the anonymous-type detail property isn't shadowed.

Skipped (Task.WhenAll parallelization, eanzhao non-blocking #4): kept sequential. Add latency at create-time is bounded; sequential is simpler to read and predictable to fail-fast on the first repo that doesn't pass.

Tests: 583 pass on Aevatar.GAgents.ChannelRuntime.Tests (up from 581 — 2 new tests for the unknown-422 fall-through and the repo-scoped 422 fail-fast paths).

[eanzhao]

Review follow-up verified on commit f77d83f — all feedback addressed:

1. validation_failed reason_code — New test FailsClosed_When_GithubSearchReturns422_WithUnknown422Body covers the fall-through branch. ✅
2. Rate-limit short-circuit guard — 403 test now asserts handler.Requests contains no /proxy/s/api-github/search/ calls, pinning that step 2+3 are never reached on step 1 failure. ✅
3. {username} literal in hint — Replaced with "the bound github_username (see field above)". ✅
4. detail shadow — Renamed to envelopeMessage at call sites in both classifiers. ✅
5. Codex P1: repo-scoped probes — Step 3 loops IReadOnlyList<string> repositories with repo:{owner}/{repo}+author:{user} queries. DailyReportTemplateSpec gains Repositories field. New test FailsClosed_When_RepoScopedSearchReturns422 pins the behavior. ✅

No remaining blocking issues. LGTM.

[eanzhao]

PR Review: feat(lark-bot): swap typing reaction to done after reply

Scope Check: DRIFT DETECTED

Stated intent (from PR title/description): Swap Lark typing reaction from "Typing" → "DONE" after reply lands.

Delivered: The typing→DONE swap feature plus a full revert of the OwnerScope/caller-scoping infrastructure from issue #466, removal of SkillRunnerStreamingReplySink, AgentBuilderActionIds, AgentBuilderJson, AgentBuilderCardContent.FormatListAgentsResult, ICallerScopeResolver chain, IUserAgentDeliveryTargetReader, and substantial frontend deletions across studio pages.

Out-of-scope changes:

• Complete removal of OwnerScope proto message and all caller-scoping resolvers
• Deletion of SkillRunnerStreamingReplySink (591 lines) and its tests
• Removal of AgentBuilderActionIds constants (inlined into consumers)
• Removal of AgentBuilderJson shared helpers (duplicated into 3 consumers)
• Frontend studio script-draft storage cleanup
• AgentDeliveryTargetTool re-exposes nyx_api_key and owner_nyx_user_id as LLM tool parameters

---

CRITICAL Findings

[P1] (confidence: 8/10) AgentDeliveryTargetTool — nyx_api_key re-exposed to LLM

The PR adds nyx_api_key back as an LLM-overridable tool parameter in AgentDeliveryTargetTool.ParametersSchema. The issue #466 review specifically removed this to close an impersonation surface: an LLM could be prompted to supply a different API key, letting it act as a different identity for outbound delivery. The PR description doesn't acknowledge this regression.

Location: src/Aevatar.AI.ToolProviders.AgentCatalog/AgentDeliveryTargetTool.cs (new nyx_api_key and owner_nyx_user_id params)

[P1] (confidence: 9/10) QueryAllAsync in UserAgentCatalogQueryPort — hard cap at 1000, no pagination

var result = await _documentReader.QueryAsync(new ProjectionDocumentQuery { Take = 1000 }, ct);

This silently truncates at 1000 entries. The old QueryByCallerAsync had cursor-based pagination with a 5000 hard cap. AgentBuilderTool.QueryAgentsForOwnerAsync then filters client-side with .Where(x => string.IsNullOrWhiteSpace(ownerFilter) || ...), so users with >1000 agents across all owners will see silently incomplete results. Even worse, UserAgentCatalogRuntimeQueryPort.QueryAllAsync loads all credentials into a dictionary (also capped at 1000 by ProjectionDocumentQuery { Take = 1000 }), and the credential set may not align 1:1 with the document set.

Location: agents/Aevatar.GAgents.Scheduled/UserAgentCatalogQueryPort.cs:28, UserAgentCatalogRuntimeQueryPort.cs:37

[P2] (confidence: 8/10) QueryAgentsForOwnerAsync — client-side owner filter after fetching ALL agents

var entries = await queryPort.QueryAllAsync(ct);
return entries
    .Where(x => string.IsNullOrWhiteSpace(ownerFilter) || string.Equals(x.OwnerNyxUserId, ownerFilter, StringComparison.Ordinal))
    ...

The old implementation pushed the owner filter into the projection store via ProjectionDocumentFilter. Now every list_agents call fetches every non-tombstoned agent in the system and filters in-process. If ownerFilter is null/empty (fallback path), it returns ALL agents from all owners — a cross-tenant information disclosure.

Location: agents/Aevatar.GAgents.Authoring.Lark/AgentBuilderTool.cs:807-812

---

INFORMATIONAL Findings

[P2] (confidence: 7/10) AgentBuilderJson helpers duplicated into 3 files

TryReadError, TryReadString, TryReadBool, TryReadOptional are now copy-pasted into AgentBuilderCardContent, AgentBuilderCardFlow, and NyxRelayAgentBuilderFlow. The deleted AgentBuilderJson.cs comment explicitly warned about this divergence hazard. Any future fix to JSON parsing logic must be replicated in 3 places.

[P2] (confidence: 7/10) AgentBuilderActionIds constants inlined

The action ID constants are now scattered as string literals across AgentBuilderCardFlow and AgentBuilderCardContent. A typo in a button argument would silently route to a fallback branch with no compile-time signal.

[P3] (confidence: 6/10) UserAgentCatalogCommandPort.Matches now compares NyxApiKey

private static bool Matches(UserAgentCatalogEntry entry, UserAgentCatalogUpsertCommand command) =>
    ...
    && string.Equals(entry.NyxApiKey, command.NyxApiKey, StringComparison.Ordinal);

The NyxApiKey is populated from the runtime query port (which reads from the credential document). The old code explicitly excluded NyxApiKey from the match because it wasn't projected to the catalog document. If the credential read model lags behind the upsert, this match will fail and the command port will time out waiting for observation.

[P3] (confidence: 5/10) Typing→DONE race on relay path

The relay path does _ = TrySwapTypingReactionToDoneAsync(...) (fire-and-forget). If the Lark API is slow, the swap may complete after the user has already seen the message. Not a correctness bug, but worth noting — the direct-reply path correctly awaits-with-timeout via AwaitTypingReactionThenSwapAsync.

---

Positive Notes

• The typing→DONE swap design is clean: list-based discovery avoids caching reaction_id on the singleton runner, operator_type=app filter correctly preserves user-added reactions.
• The AwaitTypingReactionThenSwapAsync race guard is well-thought-out with the 2-second timeout cap.
• OnReplyDeliveredAsync hook for the streaming completion path is the right abstraction.
• Test coverage is solid: new tests cover the swap sequence, streaming path hook, relay-path registration failure graceful degradation, and pagination.
• ShouldSwapTypingReaction correctly gates on om_ prefix for Lark message IDs.

---

Verdict

The typing→DONE feature is well-implemented. The OwnerScope revert is the main concern — it reintroduces the nyx_api_key as an LLM parameter, removes store-level owner filtering, and creates a cross-tenant disclosure path when ownerFilter is empty. These need to be addressed before merging.

[eanzhao]

PR #479 Review — LGTM with minor notes

核心修复正确：/rate_limit 是 scope-light 的，无法捕获 /search/* 422 的生产故障模式。新增的三层 probe（rate_limit → global search → per-repo search）覆盖了所有 failure surface。

确认的改进

• IsErrorEnvelope 提取为公共解析方法，rate-limit 和 search classifier 统一走同一个 envelope 解析，消除了之前重复的 JSON 解析逻辑
• ClassifyGitHubSearch422Body 的保守分类策略正确：匹配到 "cannot be searched" / "do not have permission" → scope_insufficient_or_user_not_found，其他 422 → validation_failed，不猜测
• best-effort api-key revoke 在所有 preflight 失败路径上都触发，跟 fix(agent-builder): use UserService.id for api-key allowed_service_ids (#417) #418 的 cleanup 契约一致
• per-repo probe 用 per_page=1 控制 payload，Uri.EscapeDataString 只用在 username 段，repo 的 slash 保留原文 — 正确处理 GitHub 的 owner/repo 格式

测试覆盖

6 个测试 pin 了关键行为：

1. /rate_limit 403 fail-fast + 短路后续所有 /search/* 调用（关键断言：handler.Requests.Should().NotContain(r => r.Path.Contains("/search/"))）
2. /search/issues 422 production envelope shape
3. /search/commits-only 422（issues 200 但 commits 422，exercises 第二个 probe）
4. 空 200 success（不误杀合法的 "no recent activity" 用户）
5. 未知 422 body → validation_failed（保守 fallback）
6. 全局 probe 200 但 repo-qualified probe 422（新 failure mode：token 有 public_repo 但 allowlist 含 private repo）

小 note（不阻断）

PreflightGitHubProxyAsync 签名从 4 参数涨到 6 参数（加了 githubUsername + repositories）。目前还在可接受范围，但如果后续再加 probe 维度，考虑封装一个 PreflightRequest record。现在不需要改。

无阻塞项。 可以合并。

[eanzhao]

Re-review after dev merge

The three-dot diff (what this PR actually adds beyond dev) is now 5 files, +845/-14 lines — purely the typing→DONE reaction swap feature. The OwnerScope revert, SkillRunnerStreamingReplySink deletion, frontend cleanup, and other cleanup changes were merged into dev via separate PRs (#478, #483, #484, #490).

Previous critical findings — status:

Finding	Status
nyx_api_key re-exposed to LLM	Addressed in #478 (merged to dev)
QueryAllAsync hard cap at 1000	Addressed in #478 (merged to dev)
Client-side owner filter cross-tenant disclosure	Addressed in #478 (merged to dev)

Remaining feature-only review:

No new critical issues found. The typing→DONE implementation is clean:

• TrySwapTypingReactionToDoneAsync correctly uses list-based discovery with operator_type=app filter
• AwaitTypingReactionThenSwapAsync race guard with 2s timeout cap is well-designed
• OnReplyDeliveredAsync hook correctly covers the streaming completion path
• ShouldSwapTypingReaction gates on om_ prefix and Lark/Feishu platform
• Pagination with MaxListPages=10 safety cap handles misbehaving Lark responses
• Test coverage is thorough: swap sequence, streaming hook, relay registration failure, pagination, non-Lark no-op, race guard, reply-failure skip

Nit (informational, confidence 6/10):

[P3] OnReplyDeliveredAsync fire-and-forget on streaming path

In ConversationGAgent.cs, the streaming completion path calls _ = ResolveRunner().OnReplyDeliveredAsync(...) — the swap is fire-and-forget. If the Lark API is slow, the user may see the DONE reaction appear after they've already moved on. Not a correctness bug, just a UX observation. The non-streaming paths (RunLlmReplyAsync, TryHandleAgentBuilderAsync) have the same pattern for the relay path (_ = TrySwapTypingReactionToDoneAsync), so this is consistent.

Verdict: LGTM — ready to merge.