feat(source-panel): GitLab parity for the PR panel and self-hosted GitLab hosts

[kyleseaman]

Problem

The Changes panel's recent PR work was built and verified against GitHub. On GitLab it misbehaves in three ways, and self-managed GitLab is not usable at all:

1. A merge request closed while still a draft shows the wrong state. GitLab keeps draft: true on an MR after it is closed, so the tab renders the draft glyph instead of Closed. Because CI is only suppressed once a source is merged/closed (feat(changes): show PR/MR state on every source tab #406), the CI chip also keeps polling a dead MR every TTL.
2. A skipped or manual-only pipeline spins forever. The chip mapped every non-success, non-failure status to running, so a fully skipped pipeline never settles — and it contradicted the panel's own Checks tab, which already buckets skipped/manual as skipped.
3. The panel header badge and the tab glyph can disagree. stateLabel ranked draft above merged/closed while pullRequestLifecycleState does the opposite, so one closed draft MR reads "Draft" in the header and Closed on its tab.
4. A self-hosted GitLab MR URL is rejected outright — parse_source_url accepted only gitlab.com, so anyone on a self-managed instance cannot use the panel at all.

Why it matters

GitLab is a first-class provider in this panel (glab is an allowlisted provider CLI, the merge-state vocabulary is already shared), but the newest surface — the per-tab lifecycle/CI chips — was effectively GitHub-only in its correctness. A stuck spinner and a mislabeled lifecycle are the two signals a reviewer reads at a glance, and the dead-MR polling spends a glab subprocess per URL per TTL for no result. Self-managed GitLab is the common enterprise deployment; rejecting it makes the whole Changes panel unavailable to those users.

Fix (symptoms -> root cause -> change)

Chip status (_fetch_check_status, src/kiro_crew/dashboard/handlers/source_providers.py):

• Draft-after-close -> the GitLab branch tested details["draft"] before the state at all, unlike the GitHub branch which requires isDraft && state == OPEN. Draft is now only reported while the MR is opened; merged/closed win, and an unknown state (e.g. GitLab locked) still yields no state rather than a mislabeled open.
• Spinning skipped pipeline -> the chip had its own inline status mapping that diverged from _gitlab_check's buckets. Both now share _gitlab_bucket, and the new _gitlab_pipeline_signal rolls one pipeline status up exactly the way the GitHub rollup treats check conclusions: any failure fails, anything in flight runs, and a terminal non-failure — including a wholly skipped or manual pipeline — passes. (Status enum verified against the GitLab pipelines REST docs: created, waiting_for_resource, preparing, waiting_for_callback, pending, running, success, failed, canceling, canceled, skipped, manual, scheduled.)
• Two subprocesses per refresh -> the MR detail payload already carries head_pipeline (the MR's own HEAD pipeline; the legacy pipeline field is deprecated in favor of it). The chip now reads that and falls back to the /pipelines?per_page=1 call only when the field is absent, matching GitHub's one-call-per-refresh cost.

Badge precedence (website/src/components/PullRequestPanel.tsx): stateLabel now orders merged -> closed -> draft, matching pullRequestLifecycleState, and is exported so the invariant is unit-testable.

Self-hosted GitLab: the reason the old code pinned GITLAB_HOST=gitlab.com was that browser input must never choose which instance a credential-bearing CLI talks to. That constraint is kept, and the host is made an operator decision instead of a URL decision:

• New dashboard.gitlab_hosts (list[str], default empty) in DashboardConfig. github.com and gitlab.com are always accepted; any other host is accepted only when its exact host[:port] is a member of that list.
• Matching is exact: no suffix or wildcard matching, www. is not stripped (unlike gitlab.com), and a portless entry does not authorize an arbitrary port on the same host.
• _coerce_gitlab_hosts fails closed at config load: a non-list yields [], and an entry carrying a scheme, userinfo, path, wildcard, or out-of-range port is dropped rather than sanitized, so a hand-edited config cannot smuggle a different target past the exact-match check.
• Every glab invocation now takes host=ref.host and pins GITLAB_HOST to the host parse_source_url authorized for that URL. _run_json re-checks the host against the allowlist before spawn and emits a denied/host_not_allowlisted SEL event otherwise — defense in depth, so a future code path that skips URL validation is denied instead of reaching an unauthorized instance.
• GET /api/dashboard/config exposes gitlab_hosts read-only (deliberately absent from the PUT allowlist — authorizing an instance is a config-file decision, not a dashboard toggle). The client uses it only to decide which pasted links become source tabs; the backend re-validates every URL regardless.
• PullRequestLinkIndex rebuilds when the allowlist changes, so adding a host mid-session retro-detects MRs already in the transcript instead of only applying to future messages.

Security note (please review this specifically)

Adding a host to dashboard.gitlab_hosts is an explicit operator decision to let the local glab CLI, with its token, reach that host — including one that only resolves on an internal network. That is intended, operator-consented behavior, which is exactly why the allowlist is deny-by-default, config-only, never browser-supplied, and re-checked at spawn. No loopback/RFC1918 denylist is applied, because an internal-only GitLab is the whole point of the feature; the trust boundary is the operator's config file. This is a different shape from a payload-supplied host (cf. the registry-URL DNS-rebinding discussion on #297), and the threat model is documented in docs/system-specs/modules/security.md.

Config/infra signal (flagged by diff_signals.py): config-baseline.json is regenerated for the new dashboard.gitlab_hosts entries only. An unrelated pre-existing drift in the committed baseline (session.empty_response_auto_continue, which is missing from the checked-in file but present in the code) was deliberately stripped to keep this PR scoped; regenerating that key belongs in its own change.

Tests

Backend — test/test_source_providers.py (+14 cases):

• test_gitlab_chip_status_uses_head_pipeline_without_second_call — locks the one-call path.
• test_gitlab_chip_status_falls_back_to_pipelines_list — locks the fallback when head_pipeline is absent.
• test_gitlab_chip_state_precedence (parametrized) — draft only while opened; closed-draft is Closed; merged wins; locked yields no state.
• test_gitlab_pipeline_signal_matches_github_rollup (parametrized) — skipped/manual settle as passed, canceled fails, unknown-but-in-flight runs, empty yields nothing.
• test_self_hosted_gitlab_rejected_when_allowlist_empty / ..._accepted_when_allowlisted — deny-by-default, and the normalized URL keeps the self-managed host so cache keys, the external link, and the CLI pin agree.
• test_self_hosted_gitlab_matches_host_exactly (parametrized) — portless entry does not authorize a port; evil-gitlab.acme.internal, gitlab.acme.internal.evil.test, a parent-domain entry, and www.-prefixed all stay rejected.
• test_self_hosted_gitlab_still_requires_https_and_mr_path — HTTPS-only, no userinfo, MR path still required for an allowlisted host.
• test_run_json_pins_glab_to_the_allowlisted_host — asserts the child GITLAB_HOST, and that it stays gitlab.com without a host.
• test_run_json_refuses_glab_host_outside_allowlist — spawn-time denial never resolves the executable or calls the sandbox.
• test_fetch_gitlab_threads_self_hosted_host_through_every_call — every glab call in a full fetch carries the host (guards against a future call site forgetting it).

Backend — test/test_config_loader.py (+5 cases, TestGitLabHostAllowlist): default empty; case/trailing-dot normalization; scheme/path/userinfo/wildcard/bad-port entries all dropped; gitlab.com and duplicates dropped; non-list falls back to empty.

Frontend — website/src/test/pullRequestLinks.test.ts (+4 cases): a self-hosted MR is ignored with no allowlist and extracted with one; port and exact-host matrix; and a mid-session allowlist change retro-scans settled messages. website/src/test/PullRequestPanel.test.tsx (+1): badge precedence matches the tab glyph for a closed draft MR.

Manual verification

Partly N/A, partly a stated gap. The GitLab code paths are driven through _run_json/glab subprocesses, so every behavior change above is covered by unit tests that assert the exact CLI arguments, the child environment, and the normalized output.

Not verified: the end-to-end glab round trip against a real self-managed GitLab server — I have no self-hosted instance reachable from this host. What is proven locally is that the host is parsed, allowlisted, threaded to every call site, and pinned into the child environment; what is unproven is glab api behaving identically against a self-managed API base. A reviewer with an internal instance adding it to dashboard.gitlab_hosts and opening one MR would close that gap.

Local gates on this commit: pytest 17,177 passed (only this host's known-environment failures: test_dashboard_origin x3, confirmed identical on untouched main, plus the known local test_skills flat-copy case), isort, flake8, tsc, eslint (237 warnings vs the 1116 CI budget, none new in the changed files), vitest 4,502 passed. mypy reports one pre-existing vector_memory.py faiss-stub error that reproduces on untouched main.

Screenshots

N/A — no new or restyled UI. The two frontend changes are behavioral: which label string an existing badge renders for a closed draft MR, and which pasted links become source tabs. Both are asserted by unit tests; there is no new panel, component, or layout to capture.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of b24a3773e0f645272336bd6fa3e433c16f373b91 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[CODEX-REVIEWED] b24a377

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b24a3773e0f645272336bd6fa3e433c16f373b91: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of b24a3773e0f645272336bd6fa3e433c16f373b91 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Cohesive GitLab-parity fixes plus a deny-by-default, backend-enforced self-hosted allowlist that fails closed and keeps the browser out of host selection.

The new config surface is backward-compatible (empty = gitlab.com only), the trust boundary is correctly placed backend-side in _run_json (the frontend parser is display-only, so a normalization drift between the TS and Python parsers is a missing/extra chip, never an unauthorized CLI call), GITLAB_TOKEN is stripped for self-managed hosts so a gitlab.com PAT can't leak cross-host, and the TTL/generation cache is a reasonable event-driven replacement for the removed poll.

[DESIGN-REVIEWED] b24a377

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed b24a3773e0f645272336bd6fa3e433c16f373b91 — this comment is updated in place on each push.

No blocking findings.

FINDING — src/kiro_crew/dashboard/handlers/source_providers.py:1112 — _gitlab_settled_merge_state is the only remaining glab call site left without host=, so await _run_json("glab", "api", mr_api) now hits the new host_not_specified guard and raises; the enclosing except SourceProviderError: break swallows it, so the GitLab merge-state re-read is dead on every host (including gitlab.com) and a conflicting MR reports mergeable="unknown" with no conflict banner until the user manually refreshes → Fix: pass host=ref.host to that call, matching the other GitLab sites.

Verdict recorded via the action's structured output for commit b24a3773e0f645272336bd6fa3e433c16f373b91.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b24a3773e0f645272336bd6fa3e433c16f373b91: <one-sentence reason>

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging b24a3773e0f645272336bd6fa3e433c16f373b91.

Second-order review for b24a3773e0f645272336bd6fa3e433c16f373b91; this comment is updated in place on each push.

Review details

I've read both files and the full diff. Here's my assessment.

The sub-threshold set is minimal: GPT 5.6 found nothing, the design review passed with no concerns, and Opus 5 raised exactly one finding (explicitly under "no blocking findings") — the _gitlab_settled_merge_state call site not passing host=, which would trip the new required-host guard and leave the GitLab first-load merge-state re-read dead, so a conflicting MR shows mergeable="unknown" with no conflict banner until a manual refresh.

Applying the narrow bar: even taking that finding at face value, it is (1) not a one-way door — the fix is a one-line host=ref.host addition in a follow-up, with no migration, API, schema, or wire-format lock-in; and (2) not concrete harm in the operational sense — no crash, hang, data loss, security regression, or unbounded resource growth. It degrades gracefully to unknown (the finding's own words: "an unknown merge state costs one banner, never the panel") and self-corrects on the next refresh. It's a transient display regression, fully reversible later. That routes to follow-ups, not a block.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

• GitLab first-load merge-state re-read may be dead (Opus 5) — if _gitlab_settled_merge_state still calls await _run_json("glab", "api", mr_api) without host=, the new required-host guard raises and the enclosing except SourceProviderError: break swallows it, so a conflicting MR reports mergeable="unknown" with no conflict banner until a manual refresh, on every host including gitlab.com. This is a reversible display regression (graceful unknown fallback, self-heals on refresh), so it can safely wait; fix by threading host=ref.host into that call site in dashboard/handlers/source_providers.py and adding a regression test that exercises the settled-merge-state path through the real _run_json guard (the existing test_fetch_gitlab_threads_self_hosted_host_through_every_call mocks _run_json, so it would not catch a raise-then-swallow). Worth confirming whether it's a live bug or a false positive before closing.

[ARBITER-REVIEWED] b24a377

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter b24a3773e0f645272336bd6fa3e433c16f373b91: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[kyleseaman]

Round 1 disposition — head 9412293c

Both blocking findings were legitimate and are fixed. Two advisory items folded in.

🔴 Claude HIGH [BLOCK-MERGE] — dashboard-config PUT 400s on every settings save — FIXED

Verified, and the reviewer's chain is exactly right. setDash in both surfaces (website/src/pages/settings/ChatPanel.tsx:154, website/src/pages/chat/ChatSettings.tsx:143) saves with mutate({ ...dashCfg, ...patch }), and dashCfg is the raw GET response — so adding gitlab_hosts to the GET put it into every PUT body, where it failed the unknown-key check. Any dashboard toggle would have failed to save.

Fixed in src/kiro_crew/dashboard/handlers/files.py with a read_only_ignored_keys set stripped alongside deprecated_ignored_keys, before the unknown-field check. The key stays out of _allowed, so it remains unwritable — a caller still cannot authorize a GitLab instance over the API.

New test/test_dashboard_config_gitlab_hosts.py (4 cases): GET exposes the configured hosts; a full GET→PUT round trip with an unrelated toggle returns 200 and leaves gitlab_hosts untouched (the exact regression); a PUT carrying gitlab_hosts: ["gitlab.evil.test"] succeeds for the writable field but never adopts the host; and a genuinely unknown key still 400s, so the drop is not a blanket bypass.

🔴 GPT HIGH [BLOCK-MERGE] — KiroCrewConfig.load() on the event loop — FIXED

Legitimate. _allowed_gitlab_hosts() read config on every call, and it is reached from async handlers and (via state.source_links()) from slot serialization, so a slow or network-backed config dir would stall the loop.

Restructured in src/kiro_crew/dashboard/handlers/source_providers.py:

• _load_gitlab_hosts() — the blocking read, documented as never-on-the-loop.
• ensure_gitlab_hosts_loaded() — refreshes a process-cached snapshot via asyncio.to_thread, at most once per 30s TTL. Awaited by fetch_pull_request, fetch_pull_request_checks, resolve_pull_request_thread, _fetch_check_status, and the status handler before any URL validation.
• _allowed_gitlab_hosts() — now a pure cache read, safe from sync code on the loop. Empty before the first refresh, which fails closed (a self-managed URL is simply not recognized yet) rather than blocking to load.

Concurrent refreshes are intentionally unlocked: the load is idempotent and last-write-wins, so no lock is held across the thread hop. Operator edits still take effect within one TTL without a restart.

Tests: one asserting the sync accessor never triggers a load while ensure_* does exactly one to_thread hop and the TTL suppresses the second; one asserting all four async entry points refresh before parsing.

GPT MEDIUM — default-port 443 mismatch — FIXED (not deferred)

Correct, and cheap enough not to defer even though the Arbiter judged it non-blocking. An explicit :443 is now treated as absent in three places that must agree: parse_source_url candidate construction, the loader coercion (a host:443 entry normalizes to the bare host), and gitlabHostSet in website/src/utils/pullRequestLinks.ts. Backend, frontend, and config-entry forms now resolve the same URL identically. Covered by one test on each side.

Claude MEDIUM — doc typo — FIXED

"and and host pinning" → "and host pinning" in docs/system-specs/modules/learn-cron-dashboard.md. The spec section was also extended to document the cached-snapshot/off-loop refresh contract and the 443 normalization.

Claude LOW — canceling bucketed as in-flight — no change, by design

Deliberate, and I agree with the reviewer's own read that it is not a correctness bug. canceling is a transient state that resolves to canceled, so bucketing it as in-flight matches the stated "anything still in flight runs" semantics and it settles to failed on the next poll. Mapping it to failed immediately would report a terminal verdict for a pipeline still winding down. The docstring enumerates the full status set precisely so this fall-through is visible rather than accidental.

Backend Tests (Windows) (4) — not from this diff, re-running on the new head

The failure is test/test_token_auth.py::test_signing_secret_write_failure_cleans_up_incomplete_file. This PR touches no auth or token code (source_providers.py, the config loader, the dashboard-config handler, and frontend link/label logic).

What I verified rather than assumed: the three most recent completed main CI runs are green on Windows shard 4, so I am not claiming this is currently broken repo-wide. What I can point at is that the test monkeypatches the global os.write and fails the first call by counter, which makes it sensitive to what else executes in the same xdist worker — an ordering hazard rather than a signal about this diff. It was also the subject of the (now closed) #444.

This push re-runs the shard. If it reproduces on 9412293c I will treat it as a real interaction and investigate properly instead of writing it off.

Local gates on 9412293c

pytest 17,188 passed; isort, flake8, tsc, vitest clean. Five failures on this host, none related: the three test_dashboard_origin port cases (this host's live gateway env, identical on untouched main), plus test_apps_registry sandbox-availability and test_vector_memory concurrency — both of which I re-ran in isolation and they pass on this branch and on main, so they are parallel-load flakes. mypy reports one pre-existing vector_memory.py faiss-stub error that also reproduces on untouched main.

[kyleseaman]

Round 2 disposition — head 8f611bc1

All six GPT findings fixed, including the HIGH. No rebuttals this round — every finding held up.

🔴 HIGH [BLOCK-MERGE] — self-hosted requests could receive the public GitLab token — FIXED

The best finding on this PR so far, and a real credential-disclosure path I introduced: _PROVIDER_AUTH_ENV_KEYS["glab"] forwards GITLAB_TOKEN unconditionally, and once round 1 made GITLAB_HOST per-request, an operator enabling a self-managed instance would have sent their gitlab.com PAT — and every permission it carries — to that server on the first MR fetch.

_run_json now removes GITLAB_TOKEN from the allowed env keys whenever the resolved host is not gitlab.com. GLAB_CONFIG_DIR is retained deliberately: that is where glab keeps its per-host credential entry, which is scoped to the host it was created for, so self-managed instances still authenticate — just not with the ambient token. gitlab.com keeps it, since that is the host the token belongs to.

Test asserts both directions: GITLAB_TOKEN absent (and GLAB_CONFIG_DIR/GITLAB_HOST present) for a self-managed host, still present for gitlab.com.

MEDIUM — concurrent refresh could restore a revoked host — FIXED

My round-1 comment claimed concurrent refreshes were "harmless, last write wins". That was wrong, and the reviewer's chain shows why: a loader that read the pre-revocation config can complete after the post-revocation snapshot is installed, overwrite it, and reset the TTL — re-admitting a host the operator just removed for another full interval. That is a security-relevant regression window, not a benign race.

ensure_gitlab_hosts_loaded() now serializes behind _gitlab_hosts_lock with a post-acquire freshness recheck, so exactly one load runs per expiry. Revert-tested: removing the lock makes the new test fail (2 loads instead of 1), restoring it passes.

MEDIUM — cold allowlist permanently cached by sidebar source extraction — FIXED

Correct, and I missed the interaction with the per-slot memo. _pr_source_links() runs synchronously and can execute before the first off-loop load, so a self-managed URL is rejected against the empty snapshot — and the result was cached by message revision alone, so a later load never invalidated it.

_publish_gitlab_hosts() now bumps gitlab_hosts_generation() on content change, and _ChatSlot._source_links_cache keys on (message_revision, allowlist_generation). Revert-tested: pinning the generation to a constant makes the new test fail. The test also covers revocation being picked up without a message mutation.

MEDIUM — pipeline-level manual reported as passed — FIXED

Legitimate, and it corrects an overreach in my round-0 change. I had collapsed skipped and manual together to stop a spinner; but a pipeline whose status is manual is blocked on a required manual job, so green claims a passing build for one that still needs action and may be blocking the merge.

Split as the reviewer suggested: _gitlab_pipeline_signal("manual") → running, while _gitlab_check keeps job-level manual → skipped (one optional manual job among finished ones is not a blocked build). skipped still settles as passed, so the original spinner fix stands. The tradeoff is explicit in the docstring: with only running/passed/failed available, running is the least-wrong signal for a blocked pipeline.

MEDIUM — staleTime does not refresh externally edited config — FIXED

Right: staleTime marks data stale, it does not poll, so an allowlist edited on disk could go unnoticed for as long as ChatPage stays mounted. Added refetchInterval: 30_000, matching the backend snapshot TTL so both sides converge on the same cadence.

MEDIUM — seen-source persistence dropped every self-hosted URL — FIXED

Correct, and a good catch on a path I did not think about. loadSeenPullRequestLinks/persistSeenPullRequestLinks canonicalized stored URLs through parseCandidate(value) with no allowlist, so self-hosted URLs failed the check, were never persisted, and after a reload looked new again — re-opening the Changes panel.

Fixed by separating the two concerns the reviewer identified: isCanonicalStoredUrl() validates the stored URL structurally (any HTTPS host with an MR shape) because the seen set is bookkeeping, not an authorization decision — whether a host may actually be loaded is decided at extraction time and re-validated by the backend. Test asserts a self-hosted URL survives a persist→load round trip with no allowlist configured.

MEDIUM (round 1, pass 3) — non-canonical port text — FIXED

git.example:08443 passed validation but was stored verbatim while both URL APIs normalize to 8443, so the entry could never match. The loader now parses the port once and rebuilds it canonically (str(port)), dropping it entirely when it equals 443. Test covers :08443 → :8443 and :0443 → bare host.

Windows shard 3 — flake, and the shard-4 question from round 1 is now answered

This round's failure is test_credwatch_present_then_deleted_fires_revocation (shard 3). Round 1's was test_signing_secret_write_failure_cleans_up_incomplete_file (shard 4).

I said last round I would not write the shard-4 failure off without evidence. The evidence now: shard 4 passed on 9412293c — the test did not reproduce — and the failure moved to a different test on a different shard. Both are Windows file-locking cases in the auth/credwatch family, and the three most recent completed main runs are green on shards 3 and 4. A diff-caused failure would be stable, not migrate between shards. This PR touches no auth, token, or credential-watch code. Treating both as Windows flakes.

Local gates on 8f611bc1

pytest 17,194 passed; vitest 4,504; isort, flake8, tsc clean. Five failures on this host, none related: the three test_dashboard_origin port cases (this host's live-gateway env, identical on untouched main), plus test_dashboard_approval recovery-continuation and test_mcp_gateway_wedge_ping_gate — both re-run in isolation and pass, so parallel-load flakes. mypy reports the same pre-existing vector_memory.py faiss-stub error that reproduces on untouched main.

Two of the six findings this round were corrections to choices I made in earlier rounds (the "harmless race" claim and the manual-as-terminal collapse), so thanks for the persistence on both.

[kyleseaman]

Round 3 disposition — head fb228189

🔴 GPT HIGH [BLOCK-MERGE] — "self-hosted mutations default to gitlab.com" — premise does not hold on this branch, but the footgun is now removed

The specific defect is not present here. Two things I verified rather than asserted:

1. No glab call site omits host. AST check over source_providers.py for _run_json("glab", ...) calls without a host keyword returns an empty list — all eleven pass host=ref.host.
2. The named endpoints do not exist on this branch. grep for auto-merge, auto_merge, markPullRequestReadyForReview, merge_when_pipeline_succeeds, and mergeRequestSetDraft across src/ and website/src returns nothing. Auto-merge and mark-ready live in unlanded feat: enable auto-merge and mark ready for review from the source panel #442.

So there is no path today by which a self-managed MR gets mutated on gitlab.com. But the hazard the reviewer identified is real and forward-looking, and #442 is exactly the PR that would hit it: with host: str = "" defaulting to gitlab.com, a new mutation call site that forgets the argument silently targets the public instance at the same project/IID. That is a bad failure mode to leave armed for a PR already in flight.

Rather than rebut and move on, I removed the class: host is now required for glab. An omitted host raises SourceProviderError("a GitLab host is required for glab calls") with a denied/host_not_specified SEL event, before the executable is resolved or the sandbox is invoked. Public callers pass host="gitlab.com" explicitly. New test asserts the refusal and that neither the resolver nor the sandbox is reached; two existing tests that relied on the implicit default were updated to pass the host explicitly.

Net effect: #442 cannot introduce this bug on merge — a forgotten host is a loud failure instead of a wrong-instance mutation.

MEDIUM — refetchInterval drives synchronous config I/O on the loop — FIXED

Correct, and my own doing: the round-2 refetchInterval: 30_000 turned a once-per-page-load config read into a poll, and api_dashboard_config called KiroCrewConfig.load() inline. The GET now loads via asyncio.to_thread, so the polling cadence can no longer stall the loop on slow storage. (Same defect class as the round-1 HIGH — I fixed the allowlist read but left the handler that serves the same data synchronous.)

MEDIUM — generation cannot invalidate the cold snapshot at initial slot serialization — FIXED

Legitimate, and it is the real gap left by round 2. The generation key makes a stale decision invalidatable, but nothing was loading the allowlist before the first synchronous serialize_slots(), so a self-hosted chip stayed absent until some unrelated provider request happened to warm the snapshot.

src/kiro_crew/dashboard/ws.py now:

• awaits ensure_gitlab_hosts_loaded() before the first serialize_slots() on WS connect (guarded, so a config-read failure degrades to a one-round lag rather than breaking the connection), and
• re-reads it once per refresh round, calling state.push_slots_update() when gitlab_hosts_generation() changes — so an authorized or revoked host reaches the sidebar without waiting for a message mutation.

New test asserts the ordering (ensure before serialize) on connect.

Claude AI Review — harness failure, not a finding; re-running

The check failed with error_max_turns: --json-schema was provided but Claude did not return structured_output. The review ran ~35 minutes and exhausted its turn budget without emitting the structured verdict, so the gate failed closed. Its last posted verdict is for 9412293c — ✅ no blocking findings — and no comment was posted for 8f611bc1 at all. Same failure mode seen on #383. Nothing to action; the new push re-runs it.

Backend Tests (Windows) (3) — third distinct credwatch test, still flaky

This round: test_credwatch_baseline_established_immediately. Round 2: test_credwatch_present_then_deleted_fires_revocation (same shard). Round 1: test_signing_secret_write_failure_cleans_up_incomplete_file (shard 4).

Three rounds, three different tests, all in the Windows auth/credwatch family, and recent main runs are green on both shards. My standing rule for this PR was to investigate if the same test failed on the same shard twice — that has not happened; the failure keeps moving. This PR touches no auth, token, or credential-watch code. Still treating it as environmental, and still not claiming it is repo-wide.

Local gates on fb228189

pytest 17,198 passed with only the three test_dashboard_origin port cases failing (this host's live-gateway env, identical on untouched main) — the parallel-load flakes from earlier rounds did not recur. vitest 4,504; isort, flake8, tsc clean. mypy reports the same pre-existing vector_memory.py faiss-stub error present on untouched main.

Two of the three findings this round were consequences of round-2 fixes rather than the original design, which is a fair signal that each round is reaching further into the seams.

[kyleseaman]

Round 4 disposition — head 168623f0

Five of six findings fixed. One HIGH is a false positive for the third consecutive round, and I want to name the pattern rather than keep rebutting it quietly.

🔴 HIGH pass 1 — "Mandatory host breaks all GitLab auto-merge and ready mutations" — not applicable; premise re-checked

The fix instruction is to "pass host=ref.host at lines 1749, 1857, and 1899." Those lines are:

• 1749 → return web.json_response({"error": str(exc)}, status=503)
• 1857 → refreshing.append(url)
• 1899 → result["ci"] = (

None is a glab call. Verified again mechanically: an AST pass over source_providers.py for _run_json("glab", ...) calls lacking a host keyword returns [], and grep for auto-merge, auto_merge, merge_when_pipeline_succeeds, mergeRequestSetDraft, markPullRequestReadyForReview across src/ and website/src returns nothing. Auto-merge and mark-ready are in unlanded #442, not this branch.

This is the third round in a row where a HIGH has been derived from #442's code as if it were in this diff (round 3: "self-hosted mutations default to gitlab.com"; round 4: the inverse, "mandatory host breaks those mutations"). Note the two are mutually contradictory: round 3 asked me to make the host mandatory because mutations might omit it, and round 4 says making it mandatory breaks those same mutations. I implemented round 3's ask precisely because it hardens the seam #442 will land into — a forgotten host is now a loud SourceProviderError instead of a silent gitlab.com retarget. When #442 rebases onto this, its _gitlab_merge_request call will need host=ref.host; that is the intended, discoverable failure.

Not filing an override — the remaining findings were real and I'd rather the check re-run on a head where they're fixed.

🔴 HIGH pass 3 — "self-hosted responses can retarget owner mutations" — real kernel, FIXED

The cited consumer (PullRequestActions) is also #442, but the underlying defect is genuine on this branch and I'd missed it: the payload's identity was provider-echoed.

"url": details.get("web_url") or ref.url,     # GitLab
"number": details.get("iid") or ref.number,
"url": details.get("url") or ref.url,         # GitHub

The browser submits that url back to /api/source/pull-request/* for refresh and thread resolution. A compromised or hostile self-managed instance could echo web_url: https://gitlab.com/victim/repo/-/merge_requests/1, and since gitlab.com is always accepted, an owner-authenticated resolve would land on someone else's MR. Accepting self-managed hosts is exactly what moved provider responses inside the blast radius, so this is a defect this PR introduced.

Both providers now pin url/number to the validated SourceRef. Test feeds a forged web_url + iid from an allowlisted host and asserts the payload keeps the requested identity.

MEDIUM — required manual jobs roll up as passed — FIXED

Right, and a real gap in round 2's split. I separated pipeline-level manual (blocked → running) from job-level (optional → skipped), but a job with allow_failure: false is a gate, so bucketing it skipped let the Checks tab read green while the build waits on a human. _gitlab_check now buckets manual + allow_failure: false as pending. Test covers both the required and optional job.

MEDIUM — warm-up only covers WebSockets — FIXED

Correct. GET /api/chat/slots is a separate entry point into the same synchronous extraction, so a cold direct fetch omitted configured self-hosted links. api_chat_slots now performs the same guarded ensure_gitlab_hosts_loaded() before serializing.

MEDIUM — untested dashboardConfig() → index wiring — FIXED

Fair: the utility tests pass hosts explicitly, so they would stay green if ChatPage stopped reading gitlab_hosts entirely. New website/src/test/ChatPage.selfHostedSources.test.tsx renders the real ChatPage and asserts the allowlist reaches PullRequestLinkIndex.update and that the MR is extracted with it (and not without it). Revert-verified: dropping the third argument in ChatPage fails it.

MEDIUM — untested generation-change branch — FIXED

Also fair. test_owner_ws_loop_pushes_slots_when_allowlist_generation_changes drives the periodic loop, flips the generation mid-round, and asserts push_slots_update() fires. The earlier warm-up test only covered ordering on connect.

Local gates on 168623f0

pytest 17,201 passed with only the three known test_dashboard_origin port cases red (this host's live-gateway env, identical on untouched main). vitest 4,506 across 389 files; isort, flake8, tsc clean. mypy reports the same pre-existing vector_memory.py faiss-stub error present on untouched main. Windows shards were green on fb228189, which retires the credwatch flake thread from earlier rounds.

Four of the five fixes this round were consequences of earlier rounds' fixes rather than the original design — the seams are getting narrower each pass, which is a reasonable sign of convergence.

[kyleseaman]

Round 5 disposition — head bd3e962b

Both findings fixed. More importantly: Claude is right and my round-3 and round-4 rebuttals were wrong. Correcting that first, because it explains two rounds of pushback.

🔴 HIGH [BLOCK-MERGE] — mandatory host= not threaded to the three GitLab mutation call sites — FIXED, and my earlier rebuttals were incorrect

I twice rebutted this class of finding by claiming the mutation endpoints "do not exist on this branch," citing a grep for merge_when_pipeline_succeeds / mergeRequestSetDraft that returned nothing and an AST pass showing no glab call missing host.

That verification was wrong in method. #442 (auto-merge / mark-ready) merged into main at 02:07 today, so from round 3 onward those endpoints were part of the PR's effective diff — the reviewers evaluate this branch as merged into current main, while I was grepping my un-rebased worktree. My checks were internally consistent and externally false. GPT flagged this in rounds 3 and 4 and I dismissed it both times; Claude has now flagged the same thing with exact line numbers, and re-checking against the rebased tree confirms all three sites:

• _gitlab_merge_request (the pre-mutation read) — every mutation goes through it
• the PUT …/merge with merge_when_pipeline_succeeds=true
• the mergeRequestSetDraft GraphQL mutation

With host required (my round-3 hardening), each raised SourceProviderError("a GitLab host is required for glab calls") → 503 + SEL denied/host_not_specified, so both GitLab mutations were dead for every host, gitlab.com included. Claude's diagnosis of why the suite missed it is also exactly right: the existing test_auto_merge_gitlab_* / test_ready_gitlab_* tests monkeypatch _run_json wholesale.

All three now pass host=ref.host. New test_gitlab_mutations_forward_host_to_the_run_json_guard drives both entry points through the real _run_json — mocking only the sandbox and process, not the guard — and asserts the self-managed host reaches GITLAB_HOST. It also covers the pre-mutation read separately, since mocking _gitlab_merge_request for the dispatch cases would otherwise leave that site uncovered. Revert-verified per site: dropping host=ref.host from the read makes the test fail with the guard's own error, which is the failure mode that shipped.

The silver lining is that round 3's hardening worked as designed — a forgotten host became a loud, discoverable failure rather than a silent retarget of a self-managed MR onto gitlab.com. It just needed to be acted on at the merge point instead of argued with.

MEDIUM — refetchInterval turned shared same-key observers into pollers — FIXED

Legitimate, and a consequence I did not trace. React Query dedupes by key, so adding refetchInterval: 30_000 to ['dashboardConfig'] silently converted the two pre-existing observers of that same key (forkCfg, dashCfg) into pollers too, and each poll writes a dashboard_config_read entry into the tamper-evident SEL chain — roughly 2,880 audit entries per day per open tab, for a value that changes almost never.

Replaced polling with the event-driven refresh Claude suggested: push_slots_update() now carries gitlabHostsGeneration, and useWebSocket invalidates ['dashboardConfig'] only when that generation actually changes. refetchInterval is gone; staleTime stays. An allowlist edited on disk still propagates (the periodic backend refresh bumps the generation, which pushes), without the constant audit churn.

Also folded in: trailing-dot host normalization

An absolute-FQDN URL (https://gitlab.acme.internal./…) parsed to a host with a trailing dot, which could never match a loader-canonicalized allowlist entry — silently failing closed. Both parse_source_url and the frontend parseCandidate now strip it, matching _coerce_gitlab_hosts.

Rebased onto main

Also picked up main's tip: KiroGhostMark.test.tsx was failing on this branch purely from staleness (#522 fixes it by asserting the mask via server render). Now 0 behind main.

Local gates on bd3e962b

pytest 17,324 passed. vitest 4,557 across 394 files — all green after the rebase. isort, flake8, tsc clean. Five backend failures on this host, none related: the three test_dashboard_origin port cases (this host's live-gateway env, identical on untouched main), plus test_apps_registry sandbox-availability and the PTY/SIGINT case — both re-run in isolation and pass, so parallel-load artifacts. mypy reports the same pre-existing vector_memory.py faiss-stub error present on untouched main.

Thanks for the persistence on this one — two reviewers had to say it before I checked the right tree.

[kyleseaman]

Round 6 disposition — head dba6c232

All five findings fixed. The first one mattered most: it made round 5's replacement mechanism inert.

MEDIUM — gitlabHostsGeneration dropped by the WS envelope — FIXED (this one invalidated round 5's fix)

Verified and correct. _broadcast() rebuilds the slots WS message key-by-key (type, data, yolo, channelTrusted), so the field push_slots_update() added was silently discarded — and the initial connect frame never carried it either. Net effect: round 5 removed refetchInterval and replaced it with a push path that never fired, so an allowlist edited on disk would not have reached the client at all. That is worse than the polling it replaced, and the finding caught it before it shipped.

Both payloads now carry it: _broadcast forwards note.get("gitlabHostsGeneration"), and the connect frame seeds the client's baseline with gitlab_hosts_generation() so a later change registers as a change rather than a first sighting. New test_slots_broadcast_carries_gitlab_hosts_generation asserts the field survives the envelope rebuild — revert-verified: deleting the forwarded key fails it.

MEDIUM — mutation entry points parse before warming — FIXED

Correct. enable_pull_request_auto_merge and mark_pull_request_ready (both from #442) called parse_source_url with no prior ensure_gitlab_hosts_loaded(), so on a cold snapshot an allowlisted self-managed MR was rejected as an unsupported host — a 400 before any dispatch. Both now warm first. The existing entry-point ordering test was extended from 4 to 6 entry points, asserting ensure precedes parse at each.

MEDIUM — monkeypatch.undo() dropped autouse fixture isolation — FIXED

Legitimate, and my own doing last round: undo() reverts all patches on that monkeypatch, including this module's autouse fixtures, so the trailing real-_run_json exercise ran without the suite's normal isolation and audit stub. Split into its own test (test_gitlab_merge_request_read_forwards_host) with its own patches, and the docstring records why it is separate rather than folded back in.

MEDIUM — cancellation during warm-up left a socket registered — FIXED

Right: the await ensure_gitlab_hosts_loaded() sat between register_ws() and the cleanup scope, so a cancellation there would strand a registered socket with nothing to unregister it. Moved the warm-up above register_ws() — it needs only to precede the first serialize_slots(), so nothing is lost.

🔴 HIGH — GitHub identity hardening had no regression test — FIXED

Fair. Round 4 pinned url/number to the validated SourceRef on both providers, but only _fetch_gitlab got a test, leaving the GitHub pin free to be reverted silently. Added test_github_payload_identity_ignores_provider_supplied_url: a payload echoing number: 999 and a victim/repo URL must not displace the requested identity. Revert-verified — restoring details.get("url") or ref.url fails it.

Local gates on dba6c232

Fully green this round: pytest 17,332 passed, 0 failed. The three test_dashboard_origin failures that have been noise in every prior round were this host's exported KIROCREW_PORT=6776 leaking into the test env — running under env -u KIROCREW_PORT clears them, so they were never a code signal. vitest 4,557 across 394 files. isort, flake8, tsc clean. mypy reports only the pre-existing vector_memory.py faiss-stub error that reproduces on untouched main.

Every finding this round was a consequence of a round-5 fix rather than the original feature, and one of them (the dropped WS field) would have shipped a non-functioning refresh path. Worth the extra pass.

[kyleseaman]

Round 7 disposition — head 861842a7

All three findings fixed. Both HIGHs were coverage gaps around round-6 fixes; the first was also a real correctness hole.

🔴 HIGH — reconnect invalidation was restart-unsafe — FIXED

Correct, and it is a genuine hole rather than only a missing test. The generation is process-local to the gateway, so after a restart it can hand out a number equal to the one the client last saw even though the allowlist on disk changed. My round-6 comparison (prevGen !== null && prevGen !== gen) would then skip the refetch, and the client would keep a stale allowlist indefinitely — self-hosted MR links quietly stop being recognized.

Now each connection treats its first generation frame as unknown and refetches, then compares normally within that connection. That required resetting the baseline in onopen (the ref outlives reconnects, so without the reset "first frame per connection" was still "first frame ever"). Cost is one extra config fetch per connect.

New website/src/test/useWebSocketGitlabHosts.test.ts covers both directions: within one connection an unchanged generation does not refetch and a bumped one does; across a reconnect an unchanged generation still refetches. Revert-verified — removing the onopen reset fails the reconnect case specifically.

🔴 HIGH — direct slot warm-up had no regression test — FIXED

Fair. Round 4 added the warm-up to api_chat_slots, but the only tests covered the WebSocket path, leaving the direct-fetch entry point free to be reverted silently. Added TestSlotsGetWarmsGitLabAllowlist: a cold snapshot, a real GET /api/chat/slots through an aiohttp test client, asserting warm-up strictly precedes serialization and that the authorized self-hosted MR comes back in source_links. Revert-verified — deleting the warm-up block fails it.

MEDIUM — ported absolute FQDN rejected — FIXED

Right, and it is the mirror of the bug I fixed last round in the other order. entry.strip().lower().rstrip(".") only removes a trailing dot when the dot is genuinely last, so gitlab.example.:8443 kept its dot mid-string and could never match the URL-normalized gitlab.example:8443. The port is now split off before dots are stripped, then the entry is rebuilt canonically. Test covers gitlab.example.:8443 → gitlab.example:8443 and plain.example.:443 → plain.example.

Local gates on 861842a7

pytest 17,332 passed. vitest 4,559 across 395 files. isort, flake8, tsc clean. Two backend failures under -n 8, both re-run in isolation and passing (test_apps_registry sandbox availability, test_dashboard_approval recovery continuation) — parallel-load artifacts, not related. mypy reports only the pre-existing vector_memory.py faiss-stub error that reproduces on untouched main.

Note on the new PR Readiness check that went red this round: it is a composite gate, so it reports failure while GPT is red. I expect it to clear on this head along with the review; if it stays red for an independent reason I will treat it as its own finding.

[kyleseaman]

Round 8 disposition — head b9d76298

All six findings fixed: two real code defects, one real test bug, three coverage gaps.

MEDIUM — _GITLAB_HOST_RE permitted a port inside name — FIXED (real defect)

Correct and worth catching. Only the last colon is split off as the port, and the name was then validated with a pattern that itself allows a trailing port — so gitlab.example:8443:443 passed validation and was stored as gitlab.example:8443, silently authorizing a host the operator never wrote. Split into _GITLAB_HOST_NAME_RE (hostname-only) for the name; the permissive pattern is gone. Test covers gitlab.example:8443:443 and a:1:2:3 → both dropped; revert-verified.

MEDIUM — synthetic manual pipeline reported as passed — FIXED (real defect)

Right, and it contradicted round 6's own decision. When a pipeline has no returned jobs, _fetch_gitlab_checks synthesizes one row from the pipeline itself; that row carries no allow_failure, so a manual pipeline fell through to the job-level reading (skipped) and the frontend rolled it up as passed — while the chip, correctly, called the same pipeline running. New _gitlab_pipeline_as_check() marks a manual pipeline as a required gate, so the fallback keeps pipeline-level semantics. Test covers manual → pending and leaves success/skipped unchanged.

MEDIUM — asyncio.Event.set() from a worker thread — FIXED (real test bug, mine)

Legitimate. My round-2 concurrency test set the event directly inside the to_thread worker; asyncio.Event is not thread-safe and this can hang or raise under asyncio debug mode. Now marshalled with loop.call_soon_threadsafe(started.set).

🔴 HIGH — three host-forwarding workflows lacked coverage — FIXED

Fair: the mutation paths got a real-guard test in round 5, but direct checks, thread resolution, and sidebar chip status still mocked _run_json without asserting host, so removing the argument there would break them silently. Added host assertions to all five call sites across those four tests.

🔴 HIGH — absolute-FQDN URL normalization had no test — FIXED

Correct. Existing tests covered dotted config entries, not dotted URLs, so either side's normalization could be reverted and a https://gitlab.acme.internal./… link would be rejected (backend) or dropped (frontend). Added a test on each side; the frontend one is revert-verified against the rawHost normalization.

MEDIUM — cold status-endpoint warm-up untested — FIXED

Added test_status_endpoint_warms_allowlist_before_parsing_self_hosted_urls: a cold snapshot, a real POST to /api/source/pull-request/status with a self-managed URL, asserting it survives validation and reaches scheduling. Prior endpoint tests used only github.com/gitlab.com URLs, which never exercise the allowlist.

Local gates on b9d76298

pytest 17,338 passed, 0 failed. vitest 4,560 across 395 files. isort, flake8, tsc clean. mypy reports only the pre-existing vector_memory.py faiss-stub error that reproduces on untouched main.

A note on where this loop is heading

Finding counts by round: 2 → 6 → 3 → 5 → 3 → 6. The character has shifted decisively, though. Rounds 1-5 surfaced real functional defects — a GITLAB_TOKEN leak to self-managed hosts, both GitLab mutations dead at the guard, a push path that never fired. Rounds 7-8 are mostly "the fix you just made lacks a test," and each such fix adds code that generates the next round's coverage finding. Two genuine defects did come out of this round, so it was worth running, but I would treat the next round as the decision point rather than assuming further rounds keep paying for themselves.

PR Readiness is a composite commit status ("6 readiness check(s) still pending"), not an independent gate — it mirrors the aggregate and should clear with the review checks.

[kyleseaman]

Round 3 — rebased onto main (5ce109ca) to clear a CONFLICTING state. Head bf4d0eb2 → 49e36e26 (single commit preserved).

The only content conflict was in src/kiro_crew/dashboard/handlers/source_providers.py, where main had independently landed a GitLab CI-projection refactor that overlapped this branch:

• main introduced _gitlab_status_bucket (per-job display bucket) and _gitlab_aggregate_ci (authoritative pipeline→chip CI glyph, the single source of truth used by both projection paths), plus _project_state for lifecycle mapping.
• This branch had introduced near-duplicate helpers _gitlab_bucket and _gitlab_pipeline_signal predating that refactor.

Resolution — adopt main's canonical helpers, preserve this branch's genuinely unique value, drop the duplicates:

• _gitlab_check: now calls main's _gitlab_status_bucket, retaining this branch's required-manual-gate refinement (manual + allow_failure is False → pending).
• Chip path (_fetch_check_status): now uses main's _project_state + _gitlab_aggregate_ci, retaining this branch's self-hosted host=ref.host on the pipelines fallback and the head_pipeline single-call optimization.
• Removed the now-dead duplicates _gitlab_bucket and _gitlab_pipeline_signal (behaviorally subsumed by _gitlab_aggregate_ci — verified case-by-case).
• Tests: deleted the redundant test_gitlab_pipeline_signal_matches_github_rollup (covered by main's test_gitlab_aggregate_ci_vocabulary, into which I folded its unique waiting_for_callback case); repointed the one remaining _gitlab_pipeline_signal assertion at _gitlab_aggregate_ci. _gitlab_pipeline_as_check and the required-manual-gate tests are unchanged and still pass.

Gates (local, off-loop CI-parity venv): source-provider suite 240 passed; gitlab/source/dashboard slice 775 passed; isort/flake8/mypy clean (481 files); frontend tsc -b clean + 84 source-panel vitest tests pass. config-baseline.json carries only the legitimate gitlab_hosts schema additions.

[kyleseaman]

Rebase round — resolved CONFLICTING against main (prev head 49e36e26 → new head b24a3773, now 0 behind main, single commit).

GitHub marked the PR CONFLICTING after main landed the merge-state re-read work (PR #565). Two semantic conflicts, both resolved by combining both sides (no logic dropped):

• src/kiro_crew/dashboard/handlers/source_providers.py
  • Secondary fanout gather (full GitLab payload): kept this branch's per-call host=ref.host pinning on every glab api call and main's _gitlab_settled_merge_state(ref, mr_api, details) re-read task + merge_state_raw unpacking. Downstream merge_state_raw consumer (tuple-guarded fallback to _gitlab_merge_state) is main's, retained.
  • Chip-status path: kept main's _record_merge_state(result, *_gitlab_merge_state(details)) and this branch's head_pipeline fast-path + host=ref.host on the pipelines fallback.
• docs/system-specs/modules/learn-cron-dashboard.md — merged the source_providers.py spec paragraph so it now documents both the self-hosted GitLab allowlist / GITLAB_TOKEN drop / GITLAB_HOST pinning (this branch) and the lazy merge-state re-read (_MERGE_STATE_REREADS/_MERGE_STATE_REREAD_DELAY_SECS, pair-settledness) plus the chip entry now carrying {state?, ci?, mergeable?, mergeStateStatus?} (main).

Local gates green on the rebased head: 324 targeted backend tests (test_source_providers, test_dashboard_config_gitlab_hosts, test_dashboard_state_ws), isort, flake8, mypy (487 files, clean), tsc -b, vitest (4744 pass). No review findings this round — rebase only.