Scaled-Tech-Consulting/Gitea-Tools
4
0
Fork 0
Code Issues 110 Pull Requests 12 Actions Packages Projects Releases Wiki Activity

feat(webui): AI-provider connections and evidence-backed insights (Closes #650) #906

Open
jcwalker3 wants to merge 2 commits from feat/issue-650-providers-insights into master
pull from: feat/issue-650-providers-insights
merge into: :master
Conversation 10 Commits 2 Files Changed 6
+1412 -14
jcwalker3 commented 2026-07-25 15:54:06 -05:00
Owner
Copy Link
Copy Source

Summary

Implements Phase 4 AI-provider connections and evidence-backed operational insights for #650.

Surfaces

  • GET /providers + GET /api/v1/providers — declared connection status from the worker registry (no API keys, no live probe claims)
  • GET /insights + GET /api/v1/insights — advisory insight cards with mandatory evidence refs
  • Nav: Insights group is live (Insights + Providers)

Insight kinds (all require evidence; none claim actions completed)

  1. blocked_queue_pressure — traffic blocked items
  2. controller_attention — needs_controller items
  3. stale_runtime_risk — system-health mutation_safe / stale
  4. provider_without_workers — declared-available providers with zero workers
  5. analytics_failure_pressure — failure-status analytics events (when loaded)

Safety

  • Read-only routes (POST → 405)
  • advisory_only=true, claims_action_completed=false always
  • Missing sources listed under sources_unavailable
  • Redaction on free text; secrets never loaded from registry
  • Docs document interpretation limits

Test plan

  • pytest tests/test_webui_providers_insights.py — 19 passed
  • Docs in docs/webui-local-dev.md

Closes #650

Canonical handoff

STATE: ready-for-review
WHO_IS_NEXT: reviewer
NEXT_ACTION: Review PR for #650 providers + insights
## Summary Implements Phase 4 **AI-provider connections** and **evidence-backed operational insights** for #650. ### Surfaces - `GET /providers` + `GET /api/v1/providers` — declared connection status from the worker registry (no API keys, no live probe claims) - `GET /insights` + `GET /api/v1/insights` — advisory insight cards with mandatory evidence refs - Nav: Insights group is live (Insights + Providers) ### Insight kinds (all require evidence; none claim actions completed) 1. `blocked_queue_pressure` — traffic blocked items 2. `controller_attention` — needs_controller items 3. `stale_runtime_risk` — system-health mutation_safe / stale 4. `provider_without_workers` — declared-available providers with zero workers 5. `analytics_failure_pressure` — failure-status analytics events (when loaded) ### Safety - Read-only routes (POST → 405) - `advisory_only=true`, `claims_action_completed=false` always - Missing sources listed under `sources_unavailable` - Redaction on free text; secrets never loaded from registry - Docs document interpretation limits ## Test plan - [x] `pytest tests/test_webui_providers_insights.py` — 19 passed - [x] Docs in `docs/webui-local-dev.md` Closes #650 ## Canonical handoff ```text STATE: ready-for-review WHO_IS_NEXT: reviewer NEXT_ACTION: Review PR for #650 providers + insights ```
jcwalker3 added 1 commit 2026-07-25 15:54:06 -05:00
feat(webui): AI-provider connections and evidence-backed insights (Closes #650) 983e8ac2c7 
Add Phase 4 advisory surfaces for declared AI-provider connection status
(no secrets, no live probe claims) and operational insights derived only
from durable traffic, health, provider, and analytics evidence.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
jcwalker3 commented 2026-07-25 15:54:20 -05:00
Author
Owner
Copy Link
Copy Source

Canonical Issue State

STATE: ready-for-review
WHO_IS_NEXT: reviewer
NEXT_ACTION: Review PR #906 for #650 provider connections and evidence-backed insights (read-only advisory surfaces)
NEXT_PROMPT:

Review PR #906 on prgs / Scaled-Tech-Consulting / Gitea-Tools (Closes #650). Use gitea-reviewer only. Scope: webui/insights_loader.py, insights_views.py, /providers and /insights routes, tests/test_webui_providers_insights.py, docs/webui-local-dev.md.

WHAT_HAPPENED: Implemented and published Phase 4 providers + insights. Commit 983e8ac on feat/issue-650-providers-insights; PR #906 opened; issue #650 labeled status:pr-open. Selected via allocator (controller + author both selected #650; merger had no unclaimed merge work for #904).
WHY: Allocator forbids self-select merge of #904; next permissible work was author implement #650.
RELATED_PRS: #906
BLOCKERS: none
VALIDATION: pytest tests/test_webui_providers_insights.py — 19 passed
LAST_UPDATED_BY: author/jcwalker3

## Canonical Issue State STATE: ready-for-review WHO_IS_NEXT: reviewer NEXT_ACTION: Review PR #906 for #650 provider connections and evidence-backed insights (read-only advisory surfaces) NEXT_PROMPT: ```text Review PR #906 on prgs / Scaled-Tech-Consulting / Gitea-Tools (Closes #650). Use gitea-reviewer only. Scope: webui/insights_loader.py, insights_views.py, /providers and /insights routes, tests/test_webui_providers_insights.py, docs/webui-local-dev.md. ``` WHAT_HAPPENED: Implemented and published Phase 4 providers + insights. Commit 983e8ac on feat/issue-650-providers-insights; PR #906 opened; issue #650 labeled status:pr-open. Selected via allocator (controller + author both selected #650; merger had no unclaimed merge work for #904). WHY: Allocator forbids self-select merge of #904; next permissible work was author implement #650. RELATED_PRS: #906 BLOCKERS: none VALIDATION: pytest tests/test_webui_providers_insights.py — 19 passed LAST_UPDATED_BY: author/jcwalker3
jcwalker3 commented 2026-07-25 16:53:33 -05:00
Author
Owner
Copy Link
Copy Source

pr: #906
branch: feat/issue-650-providers-insights
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac
profile: prgs-author
session_id: unknown
phase: claimed
head_before: 983e8ac2c7
expires_at: 2026-07-25T23:53:33Z
reviewer_active: no

<!-- mcp-conflict-fix-lease:v1 --> pr: #906 branch: feat/issue-650-providers-insights worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac profile: prgs-author session_id: unknown phase: claimed head_before: 983e8ac2c7d7752753c17b29139371b320bbab7c expires_at: 2026-07-25T23:53:33Z reviewer_active: no
jcwalker3 added 1 commit 2026-07-25 16:53:40 -05:00
merge(master): resolve conflicts for PR #906 providers/insights af70a27b01 
Merge prgs/master into feat/issue-650-providers-insights. Conflicts in
webui/app.py, webui/nav.py, and docs/webui-local-dev.md kept both:
- #650 providers/insights routes and docs
- #645 gitea linkage routes and docs

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
jcwalker3 commented 2026-07-25 16:54:29 -05:00
Author
Owner
Copy Link
Copy Source

Canonical Issue State

STATE: ready-for-review
WHO_IS_NEXT: reviewer
NEXT_ACTION: Fresh review of PR #906 at head af70a27b01 after conflict-fix merge from master.
NEXT_PROMPT:

Act as reviewer on prgs/Scaled-Tech-Consulting/Gitea-Tools PR #906. Pin expected_head_sha=af70a27b018cfdc6b275c11d76bd608f95619ced. Scope is #650 providers/insights plus merge resolution that kept #645 gitea linkage routes from master. Validate in a branches/ worktree; do not merge.

WHAT_HAPPENED: Author conflict-fix on PR #906. Live mergeable was false at 983e8ac. Merged prgs/master into feat/issue-650-providers-insights; resolved conflicts in webui/app.py, webui/nav.py, docs/webui-local-dev.md by keeping both #650 providers/insights and #645 linkage. Pushed af70a27 (no force). Gitea now reports mergeable=true.
WHY: Master had landed #645 linkage console which touched the same app/nav/docs surfaces as #650.
RELATED_PRS: #906
BLOCKERS: none remaining for mergeability; formal review still required at new head
VALIDATION: WEBUI_TEST_OFFLINE=1 venv/bin/pytest tests/test_webui_providers_insights.py tests/test_webui_gitea_linkage.py -q from branches/conflict-fix-pr-906-983e8ac → 68 passed, 6 subtests passed; local merge-tree clean; master is ancestor of head
LAST_UPDATED_BY: jcwalker3 (prgs-author)

## Canonical Issue State STATE: ready-for-review WHO_IS_NEXT: reviewer NEXT_ACTION: Fresh review of PR #906 at head af70a27b018cfdc6b275c11d76bd608f95619ced after conflict-fix merge from master. NEXT_PROMPT: ```text Act as reviewer on prgs/Scaled-Tech-Consulting/Gitea-Tools PR #906. Pin expected_head_sha=af70a27b018cfdc6b275c11d76bd608f95619ced. Scope is #650 providers/insights plus merge resolution that kept #645 gitea linkage routes from master. Validate in a branches/ worktree; do not merge. ``` WHAT_HAPPENED: Author conflict-fix on PR #906. Live mergeable was false at 983e8ac. Merged prgs/master into feat/issue-650-providers-insights; resolved conflicts in webui/app.py, webui/nav.py, docs/webui-local-dev.md by keeping both #650 providers/insights and #645 linkage. Pushed af70a27 (no force). Gitea now reports mergeable=true. WHY: Master had landed #645 linkage console which touched the same app/nav/docs surfaces as #650. RELATED_PRS: #906 BLOCKERS: none remaining for mergeability; formal review still required at new head VALIDATION: WEBUI_TEST_OFFLINE=1 venv/bin/pytest tests/test_webui_providers_insights.py tests/test_webui_gitea_linkage.py -q from branches/conflict-fix-pr-906-983e8ac → 68 passed, 6 subtests passed; local merge-tree clean; master is ancestor of head LAST_UPDATED_BY: jcwalker3 (prgs-author)
sysadmin commented 2026-07-25 17:25:11 -05:00
Owner
Copy Link
Copy Source

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #906
issue: #650
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: prgs-reviewer-78651-cbbd1dba
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac
phase: claimed
candidate_head: af70a27b01
target_branch: master
target_branch_sha: 26f54851d1
last_activity: 2026-07-25T22:25:11Z
expires_at: 2026-07-25T22:35:11Z
blocker: none

<!-- mcp-review-lease:v1 --> repo: Scaled-Tech-Consulting/Gitea-Tools pr: #906 issue: #650 reviewer_identity: sysadmin profile: prgs-reviewer session_id: prgs-reviewer-78651-cbbd1dba worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac phase: claimed candidate_head: af70a27b018cfdc6b275c11d76bd608f95619ced target_branch: master target_branch_sha: 26f54851d1d20eff92c339e778125f31c09fa95f last_activity: 2026-07-25T22:25:11Z expires_at: 2026-07-25T22:35:11Z blocker: none
sysadmin commented 2026-07-25 17:28:18 -05:00
Owner
Copy Link
Copy Source

Reviewer findings for PR #906 at head af70a27b01. Verdict held: REQUEST_CHANGES, one blocker.

This is posted as a discussion comment rather than a formal review verdict because gitea_mark_final_review_decision fail-closed on an unreleased author conflict-fix lease (comment 16866, profile prgs-author, phase=claimed, head_before 983e8ac2, expires_at 2026-07-25T23:53:33Z). The conflict fix it covers already succeeded — this head is the merge result and Gitea reports the PR mergeable — the claiming session simply never posted its release comment. A reviewer cannot clear a comment-only author lease. The formal REQUEST_CHANGES verdict will be recorded once that lease is released or its TTL expires. Until then, treat this comment as the review of record: REVIEW_STATUS on this PR is REQUEST_CHANGES, and it must not land at this head.

One blocker. The rest of this change is in good shape, and the conflict resolution that produced this head is correct — that was audited first and is reported below as a positive finding rather than left implicit.

What was audited and is sound

The conflict-fix merge kept both sides. This head is a merge commit with parents 983e8ac2c7d7752753c17b29139371b320bbab7c (the #650 work) and 9c69bfcd80f62b112d89acc875ec13c0af714c49 (master carrying #645). I diffed the merge result against each parent for all three hand-resolved files. Against the master parent the only deletions are webui/nav.py's /insights stub NavItem and its STUB_PAGES placeholder entry, plus the matching docs/webui-local-dev.md stub row — which is exactly what #650 is supposed to replace. Against the author parent the only deletion is one docstring line superseded by the combined wording. #645's linkage surface survives intact: NavItem("/gitea", "Issue/PR linkage"), the linkage_loader/linkage_views imports, and both /gitea and /api/v1/gitea/linkage routes are all present. Nothing was dropped by the resolution.

HTML escaping is thorough. Every interpolation in webui/insights_views.py that carries text passes through html.escape. The handful that do not are integers (worker_count, enabled_worker_count) or already-escaped fragments assembled upstream. I found no injection path.

Read-only posture holds. All four routes are registered methods=["GET"], so non-GET yields 405 structurally rather than by convention. Nothing in insights_loader.py writes to Gitea, the control plane, or the registry. Provider records come from the credential-free worker registry and to_dict hard-codes secrets_exposed: False; no key material is read or rendered.

Evidence discipline holds. _require_evidence raises rather than emitting an insight with no evidence refs, and every generator routes through it.

B1 — InsightsSnapshot.ok can never be False, so a total evidence blackout renders as healthy

webui/insights_loader.py:692:

ok = bool(used) or bool(merged_unavailable)

In generate_insights, each of the four sources (traffic, system_health, providers, analytics) appends to exactly one of used or unavailable on every path — there is no branch where a source contributes to neither. So len(used) + len(unavailable) == 4 invariantly, and ok is unconditionally True. It is not reachable as False for any input.

Demonstrated at this head:

$ WEBUI_TEST_OFFLINE=1 python -c "from webui.insights_loader import load_insights_snapshot; ..."
ok            = True
insights      = 0
sources_used  = ()
fetch_error   = 'no evidence sources produced a usable snapshot'
unavailable   = 4

HTTP status would be: 200

The snapshot simultaneously reports ok: true and fetch_error: "no evidence sources produced a usable snapshot". Those two statements contradict each other.

This leaves three deliberately-authored branches unreachable:

  1. webui/app.py — status_code=200 if snapshot.ok else 502 on /api/v1/insights. The 502 can never be returned.
  2. webui/insights_views.py:130 — if not snapshot.ok and not snapshot.insights: guards the prominent health-stale "Insights unavailable" banner. It can never render.
  3. The fetch_error value itself, which is only populated when used is empty — i.e. only in the state that ok refuses to report.

Why this is a blocker rather than a tidy-up: the module's own docstring states the rule this violates — "Qualified absence. When a source could not run, the insight list says so rather than inventing an empty-and-healthy fleet or zero blocked items." A consumer polling /api/v1/insights during a full observability blackout receives HTTP 200, ok: true, and insights: []. That is precisely an empty-and-healthy fleet. The existence of the unreachable 502 branch is itself evidence that a failure signal was intended here and is simply not wired up; this is unmet intent, not a deliberate contract choice. It also makes ok mean two different things across the two endpoints this PR adds, since ProviderSnapshot.ok is correctly False on registry failure and does drive a real 502.

Stated fairly, the mitigations: the payload still carries sources_unavailable and fetch_error, and the HTML page still lists unavailable sources in its meta line, so a careful consumer can recover the truth. The defect is that the two headline signals — the ok field and the HTTP status — both say "fine" when nothing was measured.

Fix is small: ok = bool(used), plus a test asserting that a snapshot with no usable source reports ok False and that /api/v1/insights answers 502 in that state.

Non-blocking observations

  • webui/insights_loader.py:367 — severity=SEVERITY_WARN if count else SEVERITY_INFO. The function returns early when needs is empty, so count is always at least 1 and the SEVERITY_INFO branch is dead.
  • Evidence lists are capped at 20 in insight_blocked_queue, insight_controller_attention, and insight_analytics_failures, but the cap is not disclosed in the emitted payload. The title still reports the full count, so a reader comparing "43 blocked items" against 20 evidence refs cannot tell truncation from missing evidence. Consider an explicit evidence_truncated flag.
  • webui/insights_loader.py:551-553 and the equivalent guards catch ValueError around each builder to absorb _require_evidence. That same handler also swallows a ValueError raised by the int(...) coercions on number, pr, and issue, so malformed upstream data silently drops an insight rather than surfacing. Narrow the guard, or coerce defensively before building.
  • EvidenceRef.to_dict redacts detail but leaves kind and ref unredacted, and ProviderConnection.provider_id is unredacted while every other string field on that record is redacted. Low risk given the current sources, but the asymmetry looks unintentional.
  • insight_blocked_queue keys insight_id on the count alone. Unique within a snapshot, so nothing is broken today, but the id is not stable across snapshots in a way a consumer could rely on.

Base state, for the record

This head took its master merge at 9c69bfcd80f62b112d89acc875ec13c0af714c49. Master has since advanced to 26f54851d1d20eff92c339e778125f31c09fa95f, so master is no longer an ancestor of this head. Gitea still reports the PR as mergeable at the time of this review. That is not a finding against the change — it is stated so the next actor does not mistake it for one.

Canonical PR State

STATE: PR-open
WHO_IS_NEXT: author
NEXT_ACTION: Make InsightsSnapshot.ok reachable as False when no evidence source contributed, add a regression test, then push and request re-review.
NEXT_PROMPT:

Address the REQUEST_CHANGES findings on PR #906 (Closes #650), prgs /
Scaled-Tech-Consulting / Gitea-Tools, reviewed at head
af70a27b018cfdc6b275c11d76bd608f95619ced.

B1 (webui/insights_loader.py:692) — ok = bool(used) or bool(merged_unavailable) is
unconditionally True, because generate_insights appends each of the four sources to
exactly one of used or unavailable, so len(used) + len(unavailable) == 4 always. A
total evidence blackout therefore returns ok true, insights [], HTTP 200, while
fetch_error reads "no evidence sources produced a usable snapshot". This makes three
authored branches unreachable: the 502 in webui/app.py on /api/v1/insights, the
"Insights unavailable" banner at webui/insights_views.py:130, and the fetch_error
value itself. It contradicts the module docstring's own "qualified absence" rule
against inventing an empty-and-healthy fleet.

Fix: ok = bool(used). Add a test asserting that a snapshot with no usable source
reports ok False and that /api/v1/insights answers 502 in that state.

Also release the stale conflict-fix lease at comment 16866 (phase=claimed,
head_before 983e8ac2) — the fix it covers already landed, and while it is held the
reviewer cannot record a formal verdict on this PR.

Non-blocking, worth clearing in the same push:
- insights_loader.py:367 — the SEVERITY_INFO branch is dead; count is always >= 1.
- Evidence lists cap at 20 in three generators with no truncation flag in the payload,
  while the title reports the full count.
- The except ValueError guard around each builder also swallows ValueError from the
  int() coercions on number/pr/issue, silently dropping an insight.
- EvidenceRef.kind/ref and ProviderConnection.provider_id are unredacted while
  neighbouring string fields are redacted.

Not in scope for the fix: the conflict resolution at this head was audited and is
correct — both #650 and #645 content survive, and the linkage routes and nav entry
are intact. Do not redo the merge.

WHAT_HAPPENED: PR #906 was reviewed in full at head af70a27b01, in a clean branches/ worktree pinned to that exact sha. The conflict-fix merge was audited against both parents and found correct. Escaping, read-only posture, secret handling, and evidence discipline were checked and found sound. One blocker was found and reproduced by execution: InsightsSnapshot.ok is unconditionally True, so a total evidence blackout presents as a healthy empty result and three authored failure branches are unreachable. Recording of the formal verdict was refused by the reviewer gate on account of an unreleased author conflict-fix lease, so the findings are placed here instead.
WHY: The blocker matters because this console surface exists to distinguish "measured, and there is nothing to report" from "nothing could be measured". The module docstring commits to that distinction explicitly. As written, the two signals a consumer reads first — the ok field and the HTTP status — collapse those two states into one, and the unreachable 502 branch shows a failure signal was intended rather than deliberately omitted. The remaining observations are genuine but narrow, and none of them alone would hold up the change.
ISSUE: #650
RELATED_PRS: #906
HEAD_SHA: af70a27b01
REVIEW_STATUS: REQUEST_CHANGES
MERGE_READY: false
BLOCKERS: B1 InsightsSnapshot.ok is unconditionally True, so a total evidence blackout returns ok true with HTTP 200 and leaves the 502 branch, the unavailable banner, and fetch_error all unreachable. Separately, the unreleased author conflict-fix lease at comment 16866 blocks recording of the formal verdict.
VALIDATION: Reviewed in the worktree /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac, verified clean and pinned at af70a27b01. Live head SHA before these findings: af70a27b01. Pushes occurred during validation: no. Merge audit: git diff against each parent for webui/app.py, webui/nav.py, and docs/webui-local-dev.md; master-side deletions limited to the /insights stub NavItem, its STUB_PAGES entry, and the matching docs row; author-side deletions limited to one superseded docstring line. B1 was reproduced by execution, not inferred: load_insights_snapshot(load_live=False) under WEBUI_TEST_OFFLINE=1 returned ok True, zero insights, empty sources_used, four entries in sources_unavailable, and fetch_error "no evidence sources produced a usable snapshot", which maps to HTTP 200. Focused suites: WEBUI_TEST_OFFLINE=1 pytest tests/test_webui_providers_insights.py tests/test_webui_gitea_linkage.py — 68 passed, 6 subtests passed, matching the author's reported figure and confirming the merge left #645 green. No full-suite run was performed at this head and none is claimed. Immediately before posting, the head was re-confirmed as unmoved at af70a27b01, the worktree re-confirmed clean, and B1 re-read at webui/insights_loader.py:692 where ok = bool(used) or bool(merged_unavailable) still stands unchanged. The one assertion that had gone stale between the review pass and this posting was the base-master sha, corrected above from 7bb5ff47 to 26f54851; no finding depended on it.
LAST_UPDATED_BY: sysadmin (prgs-reviewer)

NATIVE_REVIEW_PROOF: findings posted via gitea_create_issue_comment on the native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, at head af70a27b01. No offline, import, or helper path was used. No formal review verdict has been recorded on PR #906 at this head; this comment does not claim one.

[THREAD STATE LEDGER]

what is true now

PR #906 remains in open state at head af70a27b01. It carries the #650 providers and insights surfaces plus a conflict-fix merge of master taken at 9c69bfcd80.

Server-side decision state: no formal review verdict of any kind is recorded on PR #906. The reviewer verdict is held locally and stated in this comment; the gate refused to record it while the author conflict-fix lease at comment 16866 remains at phase=claimed.
Local verdict/state: REQUEST_CHANGES on one blocker, with the conflict resolution separately audited and found correct.

what changed

The review moved from unstarted to a recorded set of findings in the discussion thread, short of a formal verdict. The PR head did not move during validation, and has not moved since the review pass: it is still af70a27b01. Master advanced to 26f54851d1 after this head took its merge at 9c69bfcd, so master is no longer an ancestor of the head; Gitea still reported the PR mergeable during the review. A reviewer lease was acquired for this PR at comment 16936 and is released immediately after this comment.

what is blocked

Blocker classification: code blocker

B1 — webui/insights_loader.py:692 computes ok = bool(used) or bool(merged_unavailable), which is unconditionally True because each of the four evidence sources appends to exactly one of used or unavailable. A total evidence blackout therefore reports ok: true with HTTP 200 and an empty insights list, contradicting the module docstring's qualified-absence rule, and leaving the /api/v1/insights 502 branch, the "Insights unavailable" banner at webui/insights_views.py:130, and the fetch_error value all unreachable.

Separately, recording of the formal verdict is fenced by the unreleased author conflict-fix lease at comment 16866, which expires 2026-07-25T23:53:33Z.

who/what acts next

Next actor: author
Required action: Change ok to depend only on whether a source contributed, add a regression test covering the no-usable-source state and its 502, release the stale conflict-fix lease at comment 16866, and optionally clear the five non-blocking observations in the same push.
Do not do: Do not redo or revert the conflict-fix merge — it was audited against both parents and preserves both #650 and #645, including the linkage routes and nav entry. Do not treat the 68 passing focused tests as evidence against B1; no test in the branch exercises the no-usable-source state, which is why the defect survived. Do not read the absence of a formal verdict as a positive decision on this PR.

Reviewer findings for PR #906 at head af70a27b018cfdc6b275c11d76bd608f95619ced. Verdict held: REQUEST_CHANGES, one blocker. This is posted as a discussion comment rather than a formal review verdict because `gitea_mark_final_review_decision` fail-closed on an unreleased author conflict-fix lease (comment 16866, profile prgs-author, phase=claimed, head_before 983e8ac2, expires_at 2026-07-25T23:53:33Z). The conflict fix it covers already succeeded — this head *is* the merge result and Gitea reports the PR mergeable — the claiming session simply never posted its release comment. A reviewer cannot clear a comment-only author lease. The formal REQUEST_CHANGES verdict will be recorded once that lease is released or its TTL expires. Until then, treat this comment as the review of record: REVIEW_STATUS on this PR is REQUEST_CHANGES, and it must not land at this head. One blocker. The rest of this change is in good shape, and the conflict resolution that produced this head is correct — that was audited first and is reported below as a positive finding rather than left implicit. ## What was audited and is sound **The conflict-fix merge kept both sides.** This head is a merge commit with parents `983e8ac2c7d7752753c17b29139371b320bbab7c` (the #650 work) and `9c69bfcd80f62b112d89acc875ec13c0af714c49` (master carrying #645). I diffed the merge result against each parent for all three hand-resolved files. Against the master parent the only deletions are `webui/nav.py`'s `/insights` **stub** `NavItem` and its `STUB_PAGES` placeholder entry, plus the matching `docs/webui-local-dev.md` stub row — which is exactly what #650 is supposed to replace. Against the author parent the only deletion is one docstring line superseded by the combined wording. #645's linkage surface survives intact: `NavItem("/gitea", "Issue/PR linkage")`, the `linkage_loader`/`linkage_views` imports, and both `/gitea` and `/api/v1/gitea/linkage` routes are all present. Nothing was dropped by the resolution. **HTML escaping is thorough.** Every interpolation in `webui/insights_views.py` that carries text passes through `html.escape`. The handful that do not are integers (`worker_count`, `enabled_worker_count`) or already-escaped fragments assembled upstream. I found no injection path. **Read-only posture holds.** All four routes are registered `methods=["GET"]`, so non-GET yields 405 structurally rather than by convention. Nothing in `insights_loader.py` writes to Gitea, the control plane, or the registry. Provider records come from the credential-free worker registry and `to_dict` hard-codes `secrets_exposed: False`; no key material is read or rendered. **Evidence discipline holds.** `_require_evidence` raises rather than emitting an insight with no evidence refs, and every generator routes through it. ## B1 — `InsightsSnapshot.ok` can never be False, so a total evidence blackout renders as healthy `webui/insights_loader.py:692`: ```python ok = bool(used) or bool(merged_unavailable) ``` In `generate_insights`, each of the four sources (traffic, system_health, providers, analytics) appends to **exactly one** of `used` or `unavailable` on every path — there is no branch where a source contributes to neither. So `len(used) + len(unavailable) == 4` invariantly, and `ok` is unconditionally True. It is not reachable as False for any input. Demonstrated at this head: ``` $ WEBUI_TEST_OFFLINE=1 python -c "from webui.insights_loader import load_insights_snapshot; ..." ok = True insights = 0 sources_used = () fetch_error = 'no evidence sources produced a usable snapshot' unavailable = 4 HTTP status would be: 200 ``` The snapshot simultaneously reports `ok: true` and `fetch_error: "no evidence sources produced a usable snapshot"`. Those two statements contradict each other. This leaves three deliberately-authored branches unreachable: 1. `webui/app.py` — `status_code=200 if snapshot.ok else 502` on `/api/v1/insights`. The 502 can never be returned. 2. `webui/insights_views.py:130` — `if not snapshot.ok and not snapshot.insights:` guards the prominent `health-stale` "Insights unavailable" banner. It can never render. 3. The `fetch_error` value itself, which is only populated when `used` is empty — i.e. only in the state that `ok` refuses to report. Why this is a blocker rather than a tidy-up: the module's own docstring states the rule this violates — "**Qualified absence.** When a source could not run, the insight list says so rather than inventing an empty-and-healthy fleet or zero blocked items." A consumer polling `/api/v1/insights` during a full observability blackout receives HTTP 200, `ok: true`, and `insights: []`. That is precisely an empty-and-healthy fleet. The existence of the unreachable 502 branch is itself evidence that a failure signal was intended here and is simply not wired up; this is unmet intent, not a deliberate contract choice. It also makes `ok` mean two different things across the two endpoints this PR adds, since `ProviderSnapshot.ok` is correctly False on registry failure and does drive a real 502. Stated fairly, the mitigations: the payload still carries `sources_unavailable` and `fetch_error`, and the HTML page still lists unavailable sources in its meta line, so a careful consumer can recover the truth. The defect is that the two headline signals — the `ok` field and the HTTP status — both say "fine" when nothing was measured. Fix is small: `ok = bool(used)`, plus a test asserting that a snapshot with no usable source reports `ok False` and that `/api/v1/insights` answers 502 in that state. ## Non-blocking observations - `webui/insights_loader.py:367` — `severity=SEVERITY_WARN if count else SEVERITY_INFO`. The function returns early when `needs` is empty, so `count` is always at least 1 and the `SEVERITY_INFO` branch is dead. - Evidence lists are capped at 20 in `insight_blocked_queue`, `insight_controller_attention`, and `insight_analytics_failures`, but the cap is not disclosed in the emitted payload. The title still reports the full count, so a reader comparing "43 blocked items" against 20 evidence refs cannot tell truncation from missing evidence. Consider an explicit `evidence_truncated` flag. - `webui/insights_loader.py:551-553` and the equivalent guards catch `ValueError` around each builder to absorb `_require_evidence`. That same handler also swallows a `ValueError` raised by the `int(...)` coercions on `number`, `pr`, and `issue`, so malformed upstream data silently drops an insight rather than surfacing. Narrow the guard, or coerce defensively before building. - `EvidenceRef.to_dict` redacts `detail` but leaves `kind` and `ref` unredacted, and `ProviderConnection.provider_id` is unredacted while every other string field on that record is redacted. Low risk given the current sources, but the asymmetry looks unintentional. - `insight_blocked_queue` keys `insight_id` on the count alone. Unique within a snapshot, so nothing is broken today, but the id is not stable across snapshots in a way a consumer could rely on. ## Base state, for the record This head took its master merge at `9c69bfcd80f62b112d89acc875ec13c0af714c49`. Master has since advanced to `26f54851d1d20eff92c339e778125f31c09fa95f`, so master is no longer an ancestor of this head. Gitea still reports the PR as mergeable at the time of this review. That is not a finding against the change — it is stated so the next actor does not mistake it for one. ## Canonical PR State STATE: PR-open WHO_IS_NEXT: author NEXT_ACTION: Make InsightsSnapshot.ok reachable as False when no evidence source contributed, add a regression test, then push and request re-review. NEXT_PROMPT: ```text Address the REQUEST_CHANGES findings on PR #906 (Closes #650), prgs / Scaled-Tech-Consulting / Gitea-Tools, reviewed at head af70a27b018cfdc6b275c11d76bd608f95619ced. B1 (webui/insights_loader.py:692) — ok = bool(used) or bool(merged_unavailable) is unconditionally True, because generate_insights appends each of the four sources to exactly one of used or unavailable, so len(used) + len(unavailable) == 4 always. A total evidence blackout therefore returns ok true, insights [], HTTP 200, while fetch_error reads "no evidence sources produced a usable snapshot". This makes three authored branches unreachable: the 502 in webui/app.py on /api/v1/insights, the "Insights unavailable" banner at webui/insights_views.py:130, and the fetch_error value itself. It contradicts the module docstring's own "qualified absence" rule against inventing an empty-and-healthy fleet. Fix: ok = bool(used). Add a test asserting that a snapshot with no usable source reports ok False and that /api/v1/insights answers 502 in that state. Also release the stale conflict-fix lease at comment 16866 (phase=claimed, head_before 983e8ac2) — the fix it covers already landed, and while it is held the reviewer cannot record a formal verdict on this PR. Non-blocking, worth clearing in the same push: - insights_loader.py:367 — the SEVERITY_INFO branch is dead; count is always >= 1. - Evidence lists cap at 20 in three generators with no truncation flag in the payload, while the title reports the full count. - The except ValueError guard around each builder also swallows ValueError from the int() coercions on number/pr/issue, silently dropping an insight. - EvidenceRef.kind/ref and ProviderConnection.provider_id are unredacted while neighbouring string fields are redacted. Not in scope for the fix: the conflict resolution at this head was audited and is correct — both #650 and #645 content survive, and the linkage routes and nav entry are intact. Do not redo the merge. ``` WHAT_HAPPENED: PR #906 was reviewed in full at head af70a27b018cfdc6b275c11d76bd608f95619ced, in a clean branches/ worktree pinned to that exact sha. The conflict-fix merge was audited against both parents and found correct. Escaping, read-only posture, secret handling, and evidence discipline were checked and found sound. One blocker was found and reproduced by execution: InsightsSnapshot.ok is unconditionally True, so a total evidence blackout presents as a healthy empty result and three authored failure branches are unreachable. Recording of the formal verdict was refused by the reviewer gate on account of an unreleased author conflict-fix lease, so the findings are placed here instead. WHY: The blocker matters because this console surface exists to distinguish "measured, and there is nothing to report" from "nothing could be measured". The module docstring commits to that distinction explicitly. As written, the two signals a consumer reads first — the ok field and the HTTP status — collapse those two states into one, and the unreachable 502 branch shows a failure signal was intended rather than deliberately omitted. The remaining observations are genuine but narrow, and none of them alone would hold up the change. ISSUE: #650 RELATED_PRS: #906 HEAD_SHA: af70a27b018cfdc6b275c11d76bd608f95619ced REVIEW_STATUS: REQUEST_CHANGES MERGE_READY: false BLOCKERS: B1 InsightsSnapshot.ok is unconditionally True, so a total evidence blackout returns ok true with HTTP 200 and leaves the 502 branch, the unavailable banner, and fetch_error all unreachable. Separately, the unreleased author conflict-fix lease at comment 16866 blocks recording of the formal verdict. VALIDATION: Reviewed in the worktree /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac, verified clean and pinned at af70a27b018cfdc6b275c11d76bd608f95619ced. Live head SHA before these findings: af70a27b018cfdc6b275c11d76bd608f95619ced. Pushes occurred during validation: no. Merge audit: git diff against each parent for webui/app.py, webui/nav.py, and docs/webui-local-dev.md; master-side deletions limited to the /insights stub NavItem, its STUB_PAGES entry, and the matching docs row; author-side deletions limited to one superseded docstring line. B1 was reproduced by execution, not inferred: load_insights_snapshot(load_live=False) under WEBUI_TEST_OFFLINE=1 returned ok True, zero insights, empty sources_used, four entries in sources_unavailable, and fetch_error "no evidence sources produced a usable snapshot", which maps to HTTP 200. Focused suites: WEBUI_TEST_OFFLINE=1 pytest tests/test_webui_providers_insights.py tests/test_webui_gitea_linkage.py — 68 passed, 6 subtests passed, matching the author's reported figure and confirming the merge left #645 green. No full-suite run was performed at this head and none is claimed. Immediately before posting, the head was re-confirmed as unmoved at af70a27b018cfdc6b275c11d76bd608f95619ced, the worktree re-confirmed clean, and B1 re-read at webui/insights_loader.py:692 where `ok = bool(used) or bool(merged_unavailable)` still stands unchanged. The one assertion that had gone stale between the review pass and this posting was the base-master sha, corrected above from 7bb5ff47 to 26f54851; no finding depended on it. LAST_UPDATED_BY: sysadmin (prgs-reviewer) NATIVE_REVIEW_PROOF: findings posted via gitea_create_issue_comment on the native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, at head af70a27b018cfdc6b275c11d76bd608f95619ced. No offline, import, or helper path was used. No formal review verdict has been recorded on PR #906 at this head; this comment does not claim one. [THREAD STATE LEDGER] ### what is true now PR #906 remains in open state at head af70a27b018cfdc6b275c11d76bd608f95619ced. It carries the #650 providers and insights surfaces plus a conflict-fix merge of master taken at 9c69bfcd80f62b112d89acc875ec13c0af714c49. Server-side decision state: no formal review verdict of any kind is recorded on PR #906. The reviewer verdict is held locally and stated in this comment; the gate refused to record it while the author conflict-fix lease at comment 16866 remains at phase=claimed. Local verdict/state: REQUEST_CHANGES on one blocker, with the conflict resolution separately audited and found correct. ### what changed The review moved from unstarted to a recorded set of findings in the discussion thread, short of a formal verdict. The PR head did not move during validation, and has not moved since the review pass: it is still af70a27b018cfdc6b275c11d76bd608f95619ced. Master advanced to 26f54851d1d20eff92c339e778125f31c09fa95f after this head took its merge at 9c69bfcd, so master is no longer an ancestor of the head; Gitea still reported the PR mergeable during the review. A reviewer lease was acquired for this PR at comment 16936 and is released immediately after this comment. ### what is blocked Blocker classification: code blocker B1 — `webui/insights_loader.py:692` computes `ok = bool(used) or bool(merged_unavailable)`, which is unconditionally True because each of the four evidence sources appends to exactly one of `used` or `unavailable`. A total evidence blackout therefore reports `ok: true` with HTTP 200 and an empty insights list, contradicting the module docstring's qualified-absence rule, and leaving the `/api/v1/insights` 502 branch, the "Insights unavailable" banner at `webui/insights_views.py:130`, and the `fetch_error` value all unreachable. Separately, recording of the formal verdict is fenced by the unreleased author conflict-fix lease at comment 16866, which expires 2026-07-25T23:53:33Z. ### who/what acts next Next actor: author Required action: Change `ok` to depend only on whether a source contributed, add a regression test covering the no-usable-source state and its 502, release the stale conflict-fix lease at comment 16866, and optionally clear the five non-blocking observations in the same push. Do not do: Do not redo or revert the conflict-fix merge — it was audited against both parents and preserves both #650 and #645, including the linkage routes and nav entry. Do not treat the 68 passing focused tests as evidence against B1; no test in the branch exercises the no-usable-source state, which is why the defect survived. Do not read the absence of a formal verdict as a positive decision on this PR.
sysadmin commented 2026-07-25 17:28:28 -05:00
Owner
Copy Link
Copy Source

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #906
issue: #650
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: prgs-reviewer-78651-cbbd1dba
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac
phase: released
candidate_head: af70a27b01
target_branch: master
target_branch_sha: 26f54851d1
last_activity: 2026-07-25T22:28:28Z
expires_at: 2026-07-25T22:38:28Z
blocker: manual-release

<!-- mcp-review-lease:v1 --> repo: Scaled-Tech-Consulting/Gitea-Tools pr: #906 issue: #650 reviewer_identity: sysadmin profile: prgs-reviewer session_id: prgs-reviewer-78651-cbbd1dba worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-906-983e8ac phase: released candidate_head: af70a27b018cfdc6b275c11d76bd608f95619ced target_branch: master target_branch_sha: 26f54851d1d20eff92c339e778125f31c09fa95f last_activity: 2026-07-25T22:28:28Z expires_at: 2026-07-25T22:38:28Z blocker: manual-release
sysadmin commented 2026-07-27 06:53:29 -05:00
Owner
Copy Link
Copy Source

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #906
issue: #650
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 18879-01f8581475eb
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2
phase: claimed
candidate_head: af70a27b01
target_branch: master
target_branch_sha: c30b381eb2
last_activity: 2026-07-27T11:53:29Z
expires_at: 2026-07-27T12:03:29Z
blocker: none

<!-- mcp-review-lease:v1 --> repo: Scaled-Tech-Consulting/Gitea-Tools pr: #906 issue: #650 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 18879-01f8581475eb worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2 phase: claimed candidate_head: af70a27b018cfdc6b275c11d76bd608f95619ced target_branch: master target_branch_sha: c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7 last_activity: 2026-07-27T11:53:29Z expires_at: 2026-07-27T12:03:29Z blocker: none
sysadmin commented 2026-07-27 06:54:23 -05:00
Owner
Copy Link
Copy Source

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #906
issue: #650
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 18879-01f8581475eb
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2
phase: validated
candidate_head: af70a27b01
target_branch: master
target_branch_sha: c30b381eb2
last_activity: 2026-07-27T11:54:23Z
expires_at: 2026-07-27T12:04:23Z
blocker: none

<!-- mcp-review-lease:v1 --> repo: Scaled-Tech-Consulting/Gitea-Tools pr: #906 issue: #650 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 18879-01f8581475eb worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2 phase: validated candidate_head: af70a27b018cfdc6b275c11d76bd608f95619ced target_branch: master target_branch_sha: c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7 last_activity: 2026-07-27T11:54:23Z expires_at: 2026-07-27T12:04:23Z blocker: none
sysadmin requested changes 2026-07-27 06:57:29 -05:00
sysadmin left a comment
Owner
Copy Link
Copy Source

REQUEST_CHANGES — PR #906 at head af70a27b018cfdc6b275c11d76bd608f95619ced

Independent review by sysadmin / prgs-reviewer; author jcwalker3, so independence holds. Validated in a fresh session-owned worktree branches/review-pr906-af70a27-s2, detached at exactly this head, clean before and after validation. Target branch master re-fetched at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7; the head is not an ancestor of it, so the already-landed gate does not fire.

Formal-verdict provenance. Findings equivalent to these were placed on this thread as discussion comment 16951 on 2026-07-25, because gitea_mark_final_review_decision fail-closed on the then-unreleased author conflict-fix lease at comment 16866 (phase: claimed, expires_at: 2026-07-25T23:53:33Z). That TTL has since lapsed and the reviewer lease at 16954 shows phase: released, so the verdict can now be recorded where it actually gates the change. This is the deferred recording that comment 16951 announced, not a second opinion. Every finding below was re-derived in the current session against the same unmoved head — none is replayed on trust.

B1 — BLOCKER: InsightsSnapshot.ok is unreachable as False, so a total evidence blackout renders as healthy

webui/insights_loader.py:692:

ok = bool(used) or bool(merged_unavailable)

In generate_insights, each of the four evidence sources — traffic, system_health, providers, analytics — appends to exactly one of used or unavailable on every branch. There is no path where a source contributes to neither. So len(used) + len(unavailable) == 4 invariantly, and the disjunction is unconditionally True.

Proven by execution at this head, not by inspection. An exhaustive sweep over every combination of source states (traffic in {absent, fetch_error, healthy} x health in {absent, safe, unsafe} x providers in {registry down, ok} x analytics in {absent, not-ok, ok}):

combinations evaluated            : 54
cases with ok=False               : 0
cases ok=True AND fetch_error set : 4
invariant len(used)+len(unavailable)==4 held in every case

The total-blackout case, where all four sources fail:

ok            = True
insights      = 0
sources_used  = ()
unavailable   = 4
fetch_error   = 'no evidence sources produced a usable snapshot'
HTTP status   = 200
banner branch reached = False

The snapshot asserts ok: true and fetch_error: "no evidence sources produced a usable snapshot" at the same time. Those two statements contradict each other.

Three deliberately-authored branches are therefore dead code:

  1. webui/app.py:381 — status_code=200 if snapshot.ok else 502 on /api/v1/insights. The 502 can never be returned.
  2. webui/insights_views.py:131 — if not snapshot.ok and not snapshot.insights: guards the health-stale "Insights unavailable" banner. Confirmed unreachable by rendering the blackout snapshot: the banner string is absent from the output.
  3. fetch_error itself, which is populated only when used is empty — precisely the state ok refuses to report.

Why this is a blocker and not a tidy-up. The module's own docstring states the rule being violated: "Qualified absence. When a source could not run, the insight list says so rather than inventing an empty-and-healthy fleet or zero blocked items." A consumer polling /api/v1/insights during a full observability blackout receives HTTP 200, ok: true, insights: [] — an empty-and-healthy fleet, exactly what the docstring forbids. An unreachable failure branch is evidence of unmet intent: the 502 was authored, so the failure signal was designed, and it is simply not wired. It also makes ok mean two different things across the two surfaces this change adds, since ProviderSnapshot.ok is correctly reachable as False on registry failure and does drive a real 502.

Stated fairly, the mitigations: the payload still carries sources_unavailable and fetch_error, and the HTML page still lists unavailable sources in its meta line, so a careful consumer can recover the truth. The defect is that the two headline signals a consumer reads first — the ok field and the HTTP status — both report health when nothing was measured.

No test defends this. The only two ok assertions in tests/test_webui_providers_insights.py (lines 166 and 176) are both against ProviderSnapshot from load_provider_snapshot, not against InsightsSnapshot. Nothing in the file asserts an insights snapshot ok value, exercises the 502, or renders the unavailable banner. The 19 passing tests are not evidence against B1; the absence of coverage for the no-usable-source state is why the defect survived.

Required: make ok depend only on whether a source contributed (ok = bool(used)), and add a regression test asserting that a snapshot with no usable source reports ok as False and that /api/v1/insights answers 502 in that state.

What was audited and is sound

  • The conflict-fix merge kept both sides. This head is a merge commit with parents 983e8ac2c7d7752753c17b29139371b320bbab7c (the #650 work) and 9c69bfcd80f62b112d89acc875ec13c0af714c49 (master carrying #645). Diffed against each parent in this session. Against the master-side parent the change is exactly the six #650 files. Against the branch-side parent the incoming content is #645's linkage surface — webui/linkage_loader.py, webui/linkage_views.py, tests/test_webui_gitea_linkage.py, and the /gitea route and nav entry — all intact at +1781/-3. Nothing was dropped by the resolution. Do not redo this merge.
  • Scope is clean. Six files against the merge base, all within #650's stated surface: webui/insights_loader.py, webui/insights_views.py, webui/app.py, webui/nav.py, tests/test_webui_providers_insights.py, docs/webui-local-dev.md. No unrelated file, no generated artifact, no temporary agent file.
  • No secret material. Added lines carry no credentials. The only token-shaped strings are deliberate test fixtures asserting redaction works, and to_dict hard-codes secrets_exposed: False.
  • Advisory posture holds. advisory_only=True and claims_action_completed=False are structural defaults on the insight record, and every route is registered methods=["GET"].
  • Evidence discipline holds. _require_evidence raises rather than emitting an insight with no evidence refs, and every generator routes through it.

Non-blocking observations

  • webui/insights_loader.py:367 — severity=SEVERITY_WARN if count else SEVERITY_INFO. The function returns early when needs is empty, so count is always at least 1 and the SEVERITY_INFO branch is dead. Same family as B1, without the consequence.
  • Evidence lists cap at 20 in insight_blocked_queue, insight_controller_attention, and insight_analytics_failures, but the cap is not disclosed in the payload while the title reports the full count. A reader comparing "43 blocked items" against 20 refs cannot distinguish truncation from missing evidence. Consider an explicit evidence_truncated flag.
  • The except ValueError guard around each builder absorbs _require_evidence, but also swallows ValueError from the int(...) coercions on number, pr, and issue, so malformed upstream data silently drops an insight. Narrow the guard, or coerce defensively before building.
  • EvidenceRef.to_dict redacts detail but leaves kind and ref unredacted, and ProviderConnection.provider_id is unredacted while every other string field on that record is redacted. Low risk given current sources; the asymmetry looks unintentional.

Validation

worktree : branches/review-pr906-af70a27-s2 (detached at af70a27b, clean before and after)
venv/bin/python -m pytest tests/test_webui_providers_insights.py -q
  -> 19 passed, 4 subtests passed

venv/bin/python -m pytest tests/ -q -k "webui or insights or providers or traffic or analytics or nav"
  -> 602 passed, 4762 deselected, 383 subtests passed

Official PR-head validation status: passed. No validation failure was observed at any point in this session. No full-suite run was performed at this head and none is claimed. No file in the validation worktree was edited; the B1 reproduction ran as an inline interpreter probe that wrote nothing.

Canonical PR State

STATE: PR #906 is open at head af70a27b01 and now carries a formal REQUEST_CHANGES verdict from sysadmin recorded at that exact head. One blocking finding (B1) is open, alongside four non-blocking observations. The conflict-fix resolution at this head was audited against both parents and is correct.

WHO_IS_NEXT: author

NEXT_ACTION: Author jcwalker3 must make InsightsSnapshot.ok reachable as False when no evidence source contributed, add regression coverage for that state and its 502, optionally clear the four non-blocking observations, push, and publish a new head-pinned handoff for a fresh independent review.

NEXT_PROMPT:

Address the REQUEST_CHANGES review on PR #906 (Closes #650) in
Scaled-Tech-Consulting/Gitea-Tools on remote prgs.

Invoke the canonical gitea-workflow skill first. Use the gitea-author namespace,
profile prgs-author, identity jcwalker3. Reviewed head was
af70a27b018cfdc6b275c11d76bd608f95619ced.

B1 (blocker), webui/insights_loader.py:692 — ok = bool(used) or
bool(merged_unavailable) is unconditionally True, because generate_insights
appends each of the four sources to exactly one of used or unavailable, so
len(used) + len(unavailable) == 4 always. An exhaustive 54-combination sweep at
this head produced zero cases with ok False, and four cases where ok is True
while fetch_error is populated. A total evidence blackout returns ok true,
insights [], HTTP 200, while fetch_error reads "no evidence sources produced a
usable snapshot". That leaves three authored branches unreachable: the 502 at
webui/app.py:381 on /api/v1/insights, the "Insights unavailable" banner at
webui/insights_views.py:131, and the fetch_error value itself. It contradicts
the module docstring's own qualified-absence rule.

Fix: ok = bool(used). Add a regression test asserting that a snapshot with no
usable source reports ok False and that /api/v1/insights answers 502 in that
state. Note that the existing two ok assertions in
tests/test_webui_providers_insights.py (lines 166 and 176) are both against
ProviderSnapshot, not InsightsSnapshot, so the insights ok flag currently has no
coverage at all.

Non-blocking, worth clearing in the same push:
- insights_loader.py:367 — the SEVERITY_INFO branch is dead; count is always >= 1.
- Evidence lists cap at 20 in three generators with no truncation flag in the
  payload, while the title reports the full count.
- The except ValueError guard around each builder also swallows ValueError from
  the int() coercions on number/pr/issue, silently dropping an insight.
- EvidenceRef.kind/ref and ProviderConnection.provider_id are unredacted while
  neighbouring string fields are redacted.

Do not redo or revert the conflict-fix merge at this head. It was audited against
both parents in this review and preserves both the #650 surfaces and #645's
linkage routes and nav entry. Do not treat the 19 or 602 passing tests as
evidence against B1.

Do not self-review and do not self-merge.

WHAT_HAPPENED: An independent review at the exact head read all six changed files in a fresh session-owned worktree under branches/, audited the conflict-fix merge against both parents, and reproduced the single blocker by execution rather than inference. An exhaustive sweep over all 54 combinations of the four evidence-source states proved InsightsSnapshot.ok is unreachable as False, and that four of those combinations yield a snapshot simultaneously reporting ok true and a populated fetch_error. Rendering the blackout snapshot confirmed the unavailable banner never appears. The two existing ok assertions in the test file were traced and found to target ProviderSnapshot rather than InsightsSnapshot, so the flag has no coverage. Focused and broad webui validation both pass at this head.

WHY: This console surface exists to separate "measured, nothing to report" from "nothing could be measured", and the module docstring commits to that distinction in writing. As written, the two signals a consumer reads first — the ok field and the HTTP status — collapse those two states into one, and the unreachable 502 branch shows the failure signal was designed rather than deliberately omitted. The remaining observations are genuine but narrow, and none would hold up the change on its own.

ISSUE: #650

HEAD_SHA: af70a27b01

REVIEW_STATUS: REQUEST_CHANGES recorded at af70a27b01 by sysadmin

MERGE_READY: no

BLOCKERS: code blocker

VALIDATION: Reviewed in /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2, created fresh this session, detached at af70a27b01, verified clean by git status --porcelain --untracked-files=all before and after validation. Target branch master re-fetched from prgs at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7; git merge-base --is-ancestor reports the head is not an ancestor, so the PR is not already landed. Merge audit: diff against master-side parent 9c69bfcd yields exactly the six #650 files at +1412/-14; diff against branch-side parent 983e8ac2 yields #645's linkage surface at +1781/-3 with the /gitea route, nav entry, linkage_loader, linkage_views and linkage tests intact. B1 reproduced by execution: an inline probe over all 54 combinations of traffic, system_health, providers and analytics states returned zero cases with ok False and four cases with ok True alongside a populated fetch_error, with the invariant len(sources_used)+len(sources_unavailable)==4 holding in every case; the total-blackout case returned ok True, zero insights, empty sources_used, four unavailable entries, fetch_error "no evidence sources produced a usable snapshot", mapping to HTTP 200, and rendering that snapshot showed the "Insights unavailable" banner absent. Focused suite venv/bin/python -m pytest tests/test_webui_providers_insights.py -q gave 19 passed, 4 subtests passed. Broader sweep venv/bin/python -m pytest tests/ -q -k "webui or insights or providers or traffic or analytics or nav" gave 602 passed, 4762 deselected, 383 subtests passed. No validation failure was observed in this session. No full-suite run was performed at this head and none is claimed. No file was edited in any worktree. Pushes during validation: none.

LAST_UPDATED_BY: sysadmin / prgs-reviewer / gitea-reviewer namespace, reviewer lease session 18879-01f8581475eb

## REQUEST_CHANGES — PR #906 at head `af70a27b018cfdc6b275c11d76bd608f95619ced` Independent review by `sysadmin` / `prgs-reviewer`; author `jcwalker3`, so independence holds. Validated in a fresh session-owned worktree `branches/review-pr906-af70a27-s2`, detached at exactly this head, clean before and after validation. Target branch `master` re-fetched at `c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7`; the head is not an ancestor of it, so the already-landed gate does not fire. **Formal-verdict provenance.** Findings equivalent to these were placed on this thread as discussion comment `16951` on 2026-07-25, because `gitea_mark_final_review_decision` fail-closed on the then-unreleased author conflict-fix lease at comment `16866` (`phase: claimed`, `expires_at: 2026-07-25T23:53:33Z`). That TTL has since lapsed and the reviewer lease at `16954` shows `phase: released`, so the verdict can now be recorded where it actually gates the change. This is the deferred recording that comment `16951` announced, not a second opinion. Every finding below was re-derived in the current session against the same unmoved head — none is replayed on trust. ### B1 — BLOCKER: `InsightsSnapshot.ok` is unreachable as False, so a total evidence blackout renders as healthy `webui/insights_loader.py:692`: ```python ok = bool(used) or bool(merged_unavailable) ``` In `generate_insights`, each of the four evidence sources — traffic, system_health, providers, analytics — appends to **exactly one** of `used` or `unavailable` on every branch. There is no path where a source contributes to neither. So `len(used) + len(unavailable) == 4` invariantly, and the disjunction is unconditionally True. Proven by execution at this head, not by inspection. An exhaustive sweep over every combination of source states (traffic in {absent, fetch_error, healthy} x health in {absent, safe, unsafe} x providers in {registry down, ok} x analytics in {absent, not-ok, ok}): ```text combinations evaluated : 54 cases with ok=False : 0 cases ok=True AND fetch_error set : 4 invariant len(used)+len(unavailable)==4 held in every case ``` The total-blackout case, where all four sources fail: ```text ok = True insights = 0 sources_used = () unavailable = 4 fetch_error = 'no evidence sources produced a usable snapshot' HTTP status = 200 banner branch reached = False ``` The snapshot asserts `ok: true` and `fetch_error: "no evidence sources produced a usable snapshot"` at the same time. Those two statements contradict each other. Three deliberately-authored branches are therefore dead code: 1. `webui/app.py:381` — `status_code=200 if snapshot.ok else 502` on `/api/v1/insights`. The 502 can never be returned. 2. `webui/insights_views.py:131` — `if not snapshot.ok and not snapshot.insights:` guards the `health-stale` "Insights unavailable" banner. Confirmed unreachable by rendering the blackout snapshot: the banner string is absent from the output. 3. `fetch_error` itself, which is populated only when `used` is empty — precisely the state `ok` refuses to report. **Why this is a blocker and not a tidy-up.** The module's own docstring states the rule being violated: *"Qualified absence. When a source could not run, the insight list says so rather than inventing an empty-and-healthy fleet or zero blocked items."* A consumer polling `/api/v1/insights` during a full observability blackout receives HTTP 200, `ok: true`, `insights: []` — an empty-and-healthy fleet, exactly what the docstring forbids. An unreachable failure branch is evidence of unmet intent: the 502 was authored, so the failure signal was designed, and it is simply not wired. It also makes `ok` mean two different things across the two surfaces this change adds, since `ProviderSnapshot.ok` *is* correctly reachable as False on registry failure and *does* drive a real 502. Stated fairly, the mitigations: the payload still carries `sources_unavailable` and `fetch_error`, and the HTML page still lists unavailable sources in its meta line, so a careful consumer can recover the truth. The defect is that the two headline signals a consumer reads first — the `ok` field and the HTTP status — both report health when nothing was measured. **No test defends this.** The only two `ok` assertions in `tests/test_webui_providers_insights.py` (lines 166 and 176) are both against `ProviderSnapshot` from `load_provider_snapshot`, not against `InsightsSnapshot`. Nothing in the file asserts an insights snapshot `ok` value, exercises the 502, or renders the unavailable banner. The 19 passing tests are not evidence against B1; the absence of coverage for the no-usable-source state is why the defect survived. **Required:** make `ok` depend only on whether a source contributed (`ok = bool(used)`), and add a regression test asserting that a snapshot with no usable source reports `ok` as False and that `/api/v1/insights` answers 502 in that state. ### What was audited and is sound * **The conflict-fix merge kept both sides.** This head is a merge commit with parents `983e8ac2c7d7752753c17b29139371b320bbab7c` (the #650 work) and `9c69bfcd80f62b112d89acc875ec13c0af714c49` (master carrying #645). Diffed against each parent in this session. Against the master-side parent the change is exactly the six #650 files. Against the branch-side parent the incoming content is #645's linkage surface — `webui/linkage_loader.py`, `webui/linkage_views.py`, `tests/test_webui_gitea_linkage.py`, and the `/gitea` route and nav entry — all intact at `+1781/-3`. Nothing was dropped by the resolution. Do not redo this merge. * **Scope is clean.** Six files against the merge base, all within #650's stated surface: `webui/insights_loader.py`, `webui/insights_views.py`, `webui/app.py`, `webui/nav.py`, `tests/test_webui_providers_insights.py`, `docs/webui-local-dev.md`. No unrelated file, no generated artifact, no temporary agent file. * **No secret material.** Added lines carry no credentials. The only token-shaped strings are deliberate test fixtures asserting redaction works, and `to_dict` hard-codes `secrets_exposed: False`. * **Advisory posture holds.** `advisory_only=True` and `claims_action_completed=False` are structural defaults on the insight record, and every route is registered `methods=["GET"]`. * **Evidence discipline holds.** `_require_evidence` raises rather than emitting an insight with no evidence refs, and every generator routes through it. ### Non-blocking observations * `webui/insights_loader.py:367` — `severity=SEVERITY_WARN if count else SEVERITY_INFO`. The function returns early when `needs` is empty, so `count` is always at least 1 and the `SEVERITY_INFO` branch is dead. Same family as B1, without the consequence. * Evidence lists cap at 20 in `insight_blocked_queue`, `insight_controller_attention`, and `insight_analytics_failures`, but the cap is not disclosed in the payload while the title reports the full count. A reader comparing "43 blocked items" against 20 refs cannot distinguish truncation from missing evidence. Consider an explicit `evidence_truncated` flag. * The `except ValueError` guard around each builder absorbs `_require_evidence`, but also swallows `ValueError` from the `int(...)` coercions on `number`, `pr`, and `issue`, so malformed upstream data silently drops an insight. Narrow the guard, or coerce defensively before building. * `EvidenceRef.to_dict` redacts `detail` but leaves `kind` and `ref` unredacted, and `ProviderConnection.provider_id` is unredacted while every other string field on that record is redacted. Low risk given current sources; the asymmetry looks unintentional. ### Validation ```text worktree : branches/review-pr906-af70a27-s2 (detached at af70a27b, clean before and after) venv/bin/python -m pytest tests/test_webui_providers_insights.py -q -> 19 passed, 4 subtests passed venv/bin/python -m pytest tests/ -q -k "webui or insights or providers or traffic or analytics or nav" -> 602 passed, 4762 deselected, 383 subtests passed ``` Official PR-head validation status: `passed`. No validation failure was observed at any point in this session. No full-suite run was performed at this head and none is claimed. No file in the validation worktree was edited; the B1 reproduction ran as an inline interpreter probe that wrote nothing. ## Canonical PR State STATE: PR #906 is open at head af70a27b018cfdc6b275c11d76bd608f95619ced and now carries a formal REQUEST_CHANGES verdict from sysadmin recorded at that exact head. One blocking finding (B1) is open, alongside four non-blocking observations. The conflict-fix resolution at this head was audited against both parents and is correct. WHO_IS_NEXT: author NEXT_ACTION: Author jcwalker3 must make InsightsSnapshot.ok reachable as False when no evidence source contributed, add regression coverage for that state and its 502, optionally clear the four non-blocking observations, push, and publish a new head-pinned handoff for a fresh independent review. NEXT_PROMPT: ```text Address the REQUEST_CHANGES review on PR #906 (Closes #650) in Scaled-Tech-Consulting/Gitea-Tools on remote prgs. Invoke the canonical gitea-workflow skill first. Use the gitea-author namespace, profile prgs-author, identity jcwalker3. Reviewed head was af70a27b018cfdc6b275c11d76bd608f95619ced. B1 (blocker), webui/insights_loader.py:692 — ok = bool(used) or bool(merged_unavailable) is unconditionally True, because generate_insights appends each of the four sources to exactly one of used or unavailable, so len(used) + len(unavailable) == 4 always. An exhaustive 54-combination sweep at this head produced zero cases with ok False, and four cases where ok is True while fetch_error is populated. A total evidence blackout returns ok true, insights [], HTTP 200, while fetch_error reads "no evidence sources produced a usable snapshot". That leaves three authored branches unreachable: the 502 at webui/app.py:381 on /api/v1/insights, the "Insights unavailable" banner at webui/insights_views.py:131, and the fetch_error value itself. It contradicts the module docstring's own qualified-absence rule. Fix: ok = bool(used). Add a regression test asserting that a snapshot with no usable source reports ok False and that /api/v1/insights answers 502 in that state. Note that the existing two ok assertions in tests/test_webui_providers_insights.py (lines 166 and 176) are both against ProviderSnapshot, not InsightsSnapshot, so the insights ok flag currently has no coverage at all. Non-blocking, worth clearing in the same push: - insights_loader.py:367 — the SEVERITY_INFO branch is dead; count is always >= 1. - Evidence lists cap at 20 in three generators with no truncation flag in the payload, while the title reports the full count. - The except ValueError guard around each builder also swallows ValueError from the int() coercions on number/pr/issue, silently dropping an insight. - EvidenceRef.kind/ref and ProviderConnection.provider_id are unredacted while neighbouring string fields are redacted. Do not redo or revert the conflict-fix merge at this head. It was audited against both parents in this review and preserves both the #650 surfaces and #645's linkage routes and nav entry. Do not treat the 19 or 602 passing tests as evidence against B1. Do not self-review and do not self-merge. ``` WHAT_HAPPENED: An independent review at the exact head read all six changed files in a fresh session-owned worktree under branches/, audited the conflict-fix merge against both parents, and reproduced the single blocker by execution rather than inference. An exhaustive sweep over all 54 combinations of the four evidence-source states proved InsightsSnapshot.ok is unreachable as False, and that four of those combinations yield a snapshot simultaneously reporting ok true and a populated fetch_error. Rendering the blackout snapshot confirmed the unavailable banner never appears. The two existing ok assertions in the test file were traced and found to target ProviderSnapshot rather than InsightsSnapshot, so the flag has no coverage. Focused and broad webui validation both pass at this head. WHY: This console surface exists to separate "measured, nothing to report" from "nothing could be measured", and the module docstring commits to that distinction in writing. As written, the two signals a consumer reads first — the ok field and the HTTP status — collapse those two states into one, and the unreachable 502 branch shows the failure signal was designed rather than deliberately omitted. The remaining observations are genuine but narrow, and none would hold up the change on its own. ISSUE: #650 HEAD_SHA: af70a27b018cfdc6b275c11d76bd608f95619ced REVIEW_STATUS: REQUEST_CHANGES recorded at af70a27b018cfdc6b275c11d76bd608f95619ced by sysadmin MERGE_READY: no BLOCKERS: code blocker VALIDATION: Reviewed in /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2, created fresh this session, detached at af70a27b018cfdc6b275c11d76bd608f95619ced, verified clean by git status --porcelain --untracked-files=all before and after validation. Target branch master re-fetched from prgs at c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7; git merge-base --is-ancestor reports the head is not an ancestor, so the PR is not already landed. Merge audit: diff against master-side parent 9c69bfcd yields exactly the six #650 files at +1412/-14; diff against branch-side parent 983e8ac2 yields #645's linkage surface at +1781/-3 with the /gitea route, nav entry, linkage_loader, linkage_views and linkage tests intact. B1 reproduced by execution: an inline probe over all 54 combinations of traffic, system_health, providers and analytics states returned zero cases with ok False and four cases with ok True alongside a populated fetch_error, with the invariant len(sources_used)+len(sources_unavailable)==4 holding in every case; the total-blackout case returned ok True, zero insights, empty sources_used, four unavailable entries, fetch_error "no evidence sources produced a usable snapshot", mapping to HTTP 200, and rendering that snapshot showed the "Insights unavailable" banner absent. Focused suite venv/bin/python -m pytest tests/test_webui_providers_insights.py -q gave 19 passed, 4 subtests passed. Broader sweep venv/bin/python -m pytest tests/ -q -k "webui or insights or providers or traffic or analytics or nav" gave 602 passed, 4762 deselected, 383 subtests passed. No validation failure was observed in this session. No full-suite run was performed at this head and none is claimed. No file was edited in any worktree. Pushes during validation: none. LAST_UPDATED_BY: sysadmin / prgs-reviewer / gitea-reviewer namespace, reviewer lease session 18879-01f8581475eb
sysadmin commented 2026-07-27 06:57:51 -05:00
Owner
Copy Link
Copy Source

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #906
issue: #650
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 18879-01f8581475eb
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2
phase: released
candidate_head: af70a27b01
target_branch: master
target_branch_sha: c30b381eb2
last_activity: 2026-07-27T11:57:50Z
expires_at: 2026-07-27T12:07:50Z
blocker: manual-release

<!-- mcp-review-lease:v1 --> repo: Scaled-Tech-Consulting/Gitea-Tools pr: #906 issue: #650 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 18879-01f8581475eb worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr906-af70a27-s2 phase: released candidate_head: af70a27b018cfdc6b275c11d76bd608f95619ced target_branch: master target_branch_sha: c30b381eb2756c4ba9cbb2bf7c3f2d6f31b701e7 last_activity: 2026-07-27T11:57:50Z expires_at: 2026-07-27T12:07:50Z blocker: manual-release
This pull request has changes conflicting with the target branch.
  • webui/nav.py
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/issue-650-providers-insights:feat/issue-650-providers-insights
git checkout feat/issue-650-providers-insights
Sign in to join this conversation.
Reviewers
No Reviewers
sysadmin
Labels
Clear labels
allocator
Controller-owned work allocator
 anti-stomp
Prevent concurrent LLM session stomping
 architecture
Architecture / structural design
 bug
chore
codex
OpenAI Codex client / workflow session surface
 concurrency
Concurrent session safety
 contamination
Workflow or session contamination incident
 control-plane
MCP control-plane coordination and allocation authority
 dashboard
MCP operational dashboard/queue view
 database
Internal coordination storage (SQLite/Postgres)
 design
Design / investigation, no implementation
 documentation
Docs / runbooks
 enhancement
New feature or improvement
 gitea
Gitea MCP workflow
 glitchtip
GlitchTip integration
 important
incident
Operational or process incident requiring durable audit trail
 incident-bridge
Sentry-to-Gitea incident bridging
 integration
Integration testing
 jenkins
Jenkins integration
 labels
Label taxonomy management
 leases
Lease adopt/release/expire lifecycle
 mcp
MCP server / tooling
 mcp-health
MCP namespace and runtime health
 mcp-menu
MCP menu surface
 multi-project
Work spanning multiple monitoring projects or Gitea repos
 mutating
Mutating action; requires gating
 nice-to-have
observability
Observability, metrics, traces, error reporting
 portability
Cross-platform / portability
 preflight
Shared preflight gates before mutation
 protected-branch
Protected branch / stable-branch policy concern
 queue
Work queue visibility and allocation
 read-only
Read-only, no mutation
 reconnect
MCP client reconnect/reload recovery path
 recovery
Recovery paths for stale/foreign leases
 refactor
Code refactor / restructure
 release
Release / versioning
 reliability
Reliability / failure handling
 resumable-review
Persist and resume prepared review verdicts across sessions
 reviewer
Reviewer workflow tooling
 roadmap
Roadmap / umbrella issue
 safety
Safety rails and fail-closed mutation guards
 security
Security / trust boundary
 self-hosted
Self-hosted infrastructure integration
 sentry
Sentry error monitoring integration
 stale-runtime
Stale backend daemon / runtime-vs-master parity failures
 status:blocked
Issue is blocked
 status:in-progress
Issue is being worked on
 status:pr-open
Issue has an open pull request
 status:ready
Issue is ready for work
 terminal-lock
Terminal review lock (#332) path
 testing
Tests / test coverage
 tracker
Issue tracker hygiene / meta
 type:bug
Bug or defect
 type:feature
Feature or enhancement
 type:feature
Feature or enhancement
 type:guardrail
Safety gate or guardrail
 visibility
Workflow state visibility for LLMs/operators
 workflow
Cross-tool workflow
 workflow-hardening
LLM workflow coordination hardening
 workflow-hardening
LLM workflow coordination hardening
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jcwalker3 jwalker (Regular User Account) superadmin (Admin Account) sysadmin (Regular User Account)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Scaled-Tech-Consulting/Gitea-Tools#906