refactor(config): split loader responsibilities

[bolichen97]

Problem / Motivation

src/kiro_crew/config/loader.py had accumulated section DTOs, schema coercion and parsing, overlay resolution, validation-adjacent helpers, persistence, migrations, caching, and the public KiroCrewConfig facade in one roughly 9.5k-line module. The coupling made otherwise local config work hard to review and made ordering and compatibility seams easy to disturb.

Why it matters

Config loading is a compatibility boundary used across the daemon, dashboard, CLI, agents, and persisted user state. Giving its cohesive responsibilities explicit module owners reduces change collisions and makes those boundaries testable while keeping existing callers and stored configurations unchanged.

What changed (motivation → approach → change)

• Extracted section dataclasses, bounds/defaults, coercion, and section parsing into config/sections.py.
• Extracted overlay merge/subtraction, known-section classification, and degraded-state resolution into config/resolution.py.
• Kept load/save, migration, caching/fingerprinting, construction order, and the compatible KiroCrewConfig facade in config/loader.py; historical loader imports are re-exported by identity.
• Added a boundary test that locks re-export identity and prevents reverse imports from the extracted modules into loader/schema/validation.
• Updated the config owning spec and the resource-limits parse-site guard for the new owner.

The split preserves config keys, defaults, migration behavior, overlay order, clamps, degraded-file behavior, cache fingerprints, exception types, and user-visible error text.

Tests

• On the current base: the whole test/test_config* family, 941 passed and 1 skipped; the boundary, tailnet-conjunction, resource-limits, and gate-side-census guards together, 165 passed.
• On earlier bases, the config owners plus their direct daemon/dashboard/CLI/agent consumers: 2,544 passed, 16 skipped (2,560 collected).
• On an earlier base, the full backend surface on local Windows: 71,243 passed, 3,106 skipped, 5 xfailed; 30 symlink-specific tests were environment-limited by Windows WinError 1314 before reaching product code.
• On an earlier base, the frontend cross-surface suite: 5,128 passed; Electron suite: 1,456 passed, 2 skipped.
• Black, isort, flake8, mypy (1,255 source files), subprocess-encoding, SDK-boundary, brand, harness-parity, loop-lock, skill-scope, testpaths, lockdown, vendor-manifest, docs, i18n, TypeScript, ESLint, jscpd, production/analyze build, and bundle-size gates passed.
• Two extracted-contract local reviews reported no findings. Every rebase retained the identical stable patch ID 7cf627f8cf714b35, so the change itself survived each base move byte-for-byte; the three config-owner blobs move only where main independently edited config/loader.py and the split had to absorb that edit.

Manual verification

N/A — this is a behavior-preserving internal boundary refactor with no UI or external-service change; public-import identity, persistence behavior, and direct consumers are covered by automated tests.

Related Issues

no linked issue: internal maintainability refactor of the existing config loader boundary.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of fd62d00d4c9cc1a797e6e882a36e38b54af48952 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real monolith split at cohesive seams, with identity-locked re-exports, one-way import guards, and retargeted single-parse-site gates — fully reversible.

The extraction is verifiably a move, not a rewrite: loader.py's only non-import additions are functions that existed in the pre-split blob, the boundary test pins every historical name to the same object (getattr(loader, name) is getattr(module, name)) while refusing to export future module internals, and the guards that named loader.py as an owner (resource-limits single parse site, tailnet-conjunction census, agent-host-contract table) were retargeted to sections.py rather than deleted. The one-gate _normalize_acp_backend invariant moved intact with the section parser. Spec updated in the same change, per the repo's same-commit rule.

[DESIGN-REVIEWED] fd62d00

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of fd62d00d4c9cc1a797e6e882a36e38b54af48952 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All lenses are run; evidence gathered. Producing the review.

First-Principles-Verdict: CONCERNS

A clean responsibility move with identity re-exports — but loader keeps two dead compat imports nobody consumes, and two doc edits ride along undeclared.

What this change ships

Intent: make a ~9.5k-line config loader reviewable by giving its cohesive responsibilities their own modules, with zero caller-visible change — a MOVE (refactor), neither fix nor feature.

1. Section DTOs, defaults, and coercion now live in config/sections.py — justified move (measured cost: 9.5k-line module; main collided with it repeatedly during this PR's own life).
2. Overlay merge, section classification, degraded-state tracking now live in config/resolution.py — justified move.
3. Every historical loader.py name still imports from loader.py, by identity — justified (525 loader-import occurrences across 241 test files counted).
4. New guard test freezes the re-export list and forbids reverse imports — justified; it is the compat promise made testable.
5. Config spec names the new owners — derived (AGENTS.md same-commit spec rule).
6. Tailnet and resource-limits parse-site guards repointed at sections.py — declared, justified.
7. Degrade-log pointer in two seam-census docs repointed from a stale loader.py:4647-4652 to acp_backends.resolve_selected_backend() — undeclared, rides along; accurate (the log is at acp_backends.py:243) and forced by the move.
8. loader.py keeps unused math and urlsplit imports under # noqa: F401 - historical loader namespace compatibility — zero consumers.

Watch

Item 8 is inherited surface: grepped loader.math|loader._urlsplit|config.loader import (math|urlsplit) across src and test — 0 hits. The "namespace compatibility" the comment claims protects nobody.

Subtractions

• Delete import math (loader.py:17) and from urllib.parse import urlsplit as _urlsplit (loader.py:29) with their noqa tags — zero counted consumers of either name via the loader namespace; the F401 gate then guards them for free.

[FIRST-PRINCIPLES-REVIEWED] fd62d00

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] fd62d00

Verdict parsed from the review's SHA-scoped output markers for commit fd62d00d4c9cc1a797e6e882a36e38b54af48952.

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

[bolichen97]

Disposition: rebutted — keep loader.math and loader._urlsplit in this behavior-preserving split.

The governing compatibility contract for this refactor is to preserve the historical kiro_crew.config.loader module namespace and public imports, including compatibility/monkeypatch seams. Both names were bound on origin/main; the pre/post split namespace inventory intentionally retained exact parity. A repository-internal grep can show that current in-tree callers do not use them, but it cannot establish that external callers do not import or patch them. Removing them would therefore be an observable cleanup outside this PR's behavior-preserving scope.

Delete import math ... and urlsplit as _urlsplit ... zero consumers.

No code change is needed for this advisory subtraction.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] fd62d00

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

[bolichen97]

Disposition: acknowledged, no code change. I audited all 184 sections re-exports against AST-resolved imports and patch targets across src and test. Only two current patches intersect the re-export set: loader.normalize_agent_model in test_subagent_coverage.py, which remains effective because subagent.py imports it lazily from the facade at call time, and loader.DEFAULT_POOL_SIZE in test_config_loader.py, which remains effective because loader.py reads that facade global at runtime. No existing patch attempts to intercept a sections-internal lookup. The facade contract guarantees historical imports and object identity; a future test that intentionally replaces a sections-internal global should target kiro_crew.config.sections., while loader-owned seams such as config_dir remain local. I am recording that targeting rule here and keeping the already-reviewed runtime SHA stable.

[bolichen97]

CodeQL disposition: rebutted as a pre-existing Main alert, not introduced by this PR. The failed check annotates src/kiro_crew/wecom/gateway.py:105, which is absent from the PR diff. That file has the identical git blob a311eb91470b686914c23ca805560f7df6c8c39a at the PR base 283625e, this head b905ddf, and current origin/main. Repository code-scanning alert #663 already records the same py/clear-text-logging-sensitive-data finding on refs/heads/main, created 2026-08-24 at base commit 283625e. The check output itself notes that unchanged alerts can be attributed when the diff is large. Per PR scope and scanner policy, I will neither modify the unrelated WeCom gateway nor dismiss the repository-level alert.

[bolichen97]

CI disposition: resolved by rebasing the owning Main repair; no out-of-scope PR change was made.

The prior head failed only at test/test_security_posture.py::TestGateSideLogRedactorSpelling::test_the_census_holds_no_slack because dashboard/handlers/files.py had 1 counted site while the Main census still said 3. Main independently reproduced the identical assertion in run 33468402827.

Main PR #7492 repaired the owning census (3 → 1) in commit ecab0babe525522d293b0da8628a1b49203e2db1. This PR is now rebased as one commit on Main 5603ae7449d7e2ade46cdd8c4b09bd86444c704c; new head 67f9a0b2f9e27e91e7ab64ede85c4c7e6beec933 therefore contains that upstream repair. The exact census test passes locally, and a fresh CI/review round is running on the new head.

The config refactor never modified src/kiro_crew/dashboard/handlers/files.py or the census value itself; it only updated the directly corresponding ownership assertion so the moved tailnet parser is recognized in config/sections.py.

[bolichen97]

span=first-principles/subtractions/loader-math-urlsplit

• Delete the two noqa: F401 stdlib imports from config/loader.py — disposition: rebutted, kept deliberately.

Delete import math # noqa: F401 and from urllib.parse import urlsplit as _urlsplit # noqa: F401 from src/kiro_crew/config/loader.py:17,29 — 0 consumers of either through the loader namespace.
Item 9 is the only surface whose zero option costs nobody anything: loader.py no longer uses either name

The lane's factual claim is correct and I re-derived every part of it rather than
asserting the opposite: loader.py has 0 hits for math. / _urlsplit(, no
from kiro_crew.config.loader import ... block names either symbol, there is no
loader.math / loader._urlsplit attribute access in the tree, and neither name
appears in the boundary test's pinned re-export list.

What the disposition turns on is that this is not a subtraction of something the PR
added. Both bindings already exist on origin/main's loader.py (lines 17 and 29
there too); this PR added only the # noqa: F401 tags, because the split moved the
last in-module uses out while the contract it sells is that the loader namespace is
unchanged. A probe that imports kiro_crew.config.loader under origin/main and under
this head reports 383/383 identical bindings — 0 added, 0 removed — and identical values
for every non-callable module constant. Deleting the two names would make that 381/383
and would be the one class of change a declared behaviour-preserving split promised not
to make: an observable narrowing of the module's namespace, which no in-repo grep can
prove is unobserved by an out-of-tree importer or monkeypatch.

The lane is advisory (CONCERNS, not BLOCK) and does not gate readiness. It is a
trivially separable follow-up: a 2-line deletion with no test change, correctly reviewed
as an intentional namespace narrowing rather than as cleanup riding along inside a
13k-line refactor.

[bolichen97]

span=codeql/alert-663/py-clear-text-logging-sensitive-data

• py/clear-text-logging-sensitive-data at src/kiro_crew/wecom/gateway.py:105 — disposition: rebutted, pre-existing on main and not reachable from this diff.

This alert is not introduced by this pull request. Unchanged alerts can be attributed to a pull request when its diff is large.

Verified over the code-scanning REST API rather than from the check summary:
GET /repos/kirodotdev/KiroCrew/code-scanning/alerts/663 returns state=open,
ref=refs/heads/main, created_at=2026-08-24T06:16:19Z, severity high,
src/kiro_crew/wecom/gateway.py:105. That path is not one of this PR's nine changed
files, and the file's git blob is identical at this head and at origin/main.

On its merits it is also a false positive: the logged missing string is built as
" and ".join(name for name, value in credentials if not value), i.e. only the names
of credentials whose value is falsy. No credential value is ever a format argument, and
the call already carries a # nosemgrep: python-logger-credential-disclosure
suppression for the same shape.

I deliberately took neither available shortcut. Adding a CodeQL suppression would widen
this refactor into an unrelated module, and dismissing a repository-level alert on main
is a maintainer action, not the author's. Clearing the red therefore needs either an admin
dismissal of alert #663 or a separate one-line PR against wecom/gateway.py; until then
the CodeQL check stays red on this PR for a defect that exists equally on main.

[bolichen97]

span=ci/backend-tests-windows-3/test_session_control-rate-limit

• Windows shard 3: two test_session_control.py cases fail with SessionControlError: too many sessions created recently; retry shortly — disposition: rebutted, not introduced by this PR.

SessionControlError: too many sessions created recently; retry shortly

The failure is a creation-rate-limit window bleeding across tests under that shard's
ordering, not a config-loading change. src/kiro_crew/dashboard/session_control.py
has an identical git blob at this head and at origin/main, the file is not among this
PR's nine changed paths, and nothing in the diff alters how a session is created or how
the rate-limit window is measured — the split moved section dataclasses and resolution
helpers out of config/loader.py while keeping every historical name importable from it
with the same object identity (pinned by test/test_config_module_boundaries.py).

Locally the whole module passes: 142 passed for test/test_session_control.py. The
failure is order- and load-dependent and Windows-shard-only, which is consistent with a
shared window rather than with a behavioural change. Fixing it means giving the
rate-limit window a per-test reset in the owning test module — a change to a file this
PR does not touch, and one that does not belong inside a behaviour-preserving config
refactor. I did not weaken the assertion or the limit to make the shard green.

[bolichen97]

span=ci/frontend-tests-4/mochi-chat-panel-unclosed-fence

• Frontend Tests (4) fails at MochiChatPanel.coverage.test.tsx > ChatPanel streaming footer > closes a code fence the stream has not finished yet, taking Coverage Gate down with it — disposition: rebutted, a runner-timing flake in a file this PR does not touch. Cleared by re-running the two jobs; no code change.

FAIL src/test/MochiChatPanel.coverage.test.tsx > ChatPanel streaming footer > closes a code fence the stream has not finished yet
TestingLibraryElementError: Unable to find an element with the text: print(1). This could be because the text is broken up by multiple elements.
src/test/MochiChatPanel.coverage.test.tsx:843:25
waitForWrapper node_modules/@testing-library/dom/dist/wait-for.js:163:27

Not this diff. The change is nine files: three docs, src/kiro_crew/config/{loader,resolution,sections}.py, and three backend tests. git diff --name-only origin/main...HEAD -- website/ is empty, and nothing in the vitest suite imports the changed modules.

Decisive comparison. Head b905ddf51d93 of this same branch produced a fully green CI run (33443286965) — Frontend Tests (1) through (4) and Coverage Gate all success. git diff b905ddf51d93 fd62d00d4c9c over the four files that decide this assertion — website/src/test/MochiChatPanel.coverage.test.tsx, website/src/apps/mochi/src/renderer/ChatPanel.tsx, website/src/apps/mochi/panel/MochiCodeBlock.tsx, website/src/utils/highlightClient.ts — is empty; they are byte-identical. Across the six prior CI runs of this branch Frontend Tests (4) never failed once: every earlier red was the backend shard-3 census regression main has since fixed, or the known Windows session-rate-limit race.

Not inherited from the base either. Frontend Tests (4) is success on the fourteen most recent completed CI runs across other branches off the same main tip, so the assertion is not deterministically broken upstream.

The mechanism is the 1000 ms findByText window. The assertion is expect(await screen.findByText('print(1)')), whose default wait is 1000 ms. The case is reported at 1038 ms and the stack ends in waitForWrapper ... wait-for.js:163 — the window expired, rather than an element being wrong or absent. That file ran 85 tests in 12,328 ms on this runner, and the shard finished 997 of 998 tests green.

The one plausible non-timing race is ruled out. MochiCodeBlock swaps plain text for hljs-tokenized HTML once highlightAsync resolves, which would indeed break print(1) across elements — but it cannot happen in this environment: the unit project runs on happy-dom (website/vite.config.ts), happy-dom ships no Worker, and the unit project declares no setupFiles that would polyfill one (the only setupFiles there belongs to the integration project). So highlightClient.getWorker() returns null, highlightAsync returns Promise.resolve(''), and the block stays plain text permanently. The immediately adjacent case in the same describe already carries a stabilization for exactly this class, commented "a slow runner still sees the pre-strip markup and fails".

Coverage Gate is the dependent symptom, from its own log, not a second defect:

coverage-combine=success frontend-test=failure frontend-coverage-merge=skipped backend-test=success only_backend=true only_frontend=false
::error::frontend-test=failure -- failing closed.

Backend tests and coverage combine both passed, and on the green head b905ddf51d93 this same diff had Coverage Gate success.

Action taken: re-ran only the failed jobs of run 33578147220. Attempt 3 re-runs Frontend Tests (4) and its dependent Coverage Gate in dependency order; the already-passing Windows shard 3 is untouched.

Deliberately not done here: stabilizing the assertion. Awaiting the stream commit before the negative, in the style of the sibling case, is correct work and belongs in a frontend PR that owns that file. Adding website/ to a behaviour-preserving backend config split would widen the diff into an unrelated tree and force a fresh full review round for a file this PR does not own.

[bolichen97]

Supplementary CodeQL evidence scoped to the current head fd62d00d4c9c. The adjudication record for this finding is the earlier target=codeql disposition comment and it still stands unchanged; this comment deliberately carries no marker so it does not duplicate that record. It adds two facts that record does not have.

1. The CodeQL lane is not a readiness blocker on this head. .github/workflows/pr-readiness.yml monitors this lane as the workflow dynamic/github-code-scanning/codeql and maps success|skipped to passed. That workflow run on fd62d00d4c9c is 33578144512, conclusion success. The red check named CodeQL is the code-scanning app's alert check-run (100088402749), which readiness never reads. The same conclusion from the other side: of readiness's nine monitored lanes on this head exactly one is failure-class — CI, run 33578147220 — while Build (33578147132), Code Review (33578321918), CodeQL, Opus 4.8, GPT 5.6, Design, UX and First Principles are all success. That matches the published 1 blocking readiness item(s) exactly, and it identifies the blocker as the frontend shard, not this alert.

2. Why this PR surfaces the alert when smaller ones do not, and why no rebase clears it. WeComConfig, the taint source on the alert's dataflow path, moves from config/loader.py:6375 on main to config/sections.py:4830 here, so a node of that path now lies in changed code. That is the concrete form of CodeQL's own caveat, "Alerts not introduced by this pull request might have been detected because the code changes were too large". Consistent with that, the lane reddened on every prior head of this branch including the otherwise fully green b905ddf51d93, while it passes on smaller contemporaneous PRs.

On the merits it remains a false positive, unchanged from the earlier record. src/kiro_crew/wecom/gateway.py returns early on if not cfg_enabled or all(value for _, value in credentials), so it only ever formats missing = " and ".join(name for name, value in credentials if not value) — the env-var NAMES of the ABSENT operands. The format arguments are channel_type, missing and settings_name; no credential value is among them, and the call already carries # nosemgrep: python-logger-credential-disclosure. Alert #663 is state=open on refs/heads/main, created 2026-08-24, at the same src/kiro_crew/wecom/gateway.py:105. The identical class was already dismissed on main as alert #630 — in src/kiro_crew/config/loader.py, the very file this PR splits — with the recorded reason "a key name/PATH, not a credential value".

I am deliberately not acting on the alert from this PR. Adding a suppression to wecom/gateway.py would widen an unrelated file inside a config refactor, and dismissing a repository security alert changes main's posture, which is a maintainer's call rather than an author's. Two options for a maintainer, my recommendation second:

(a) dismiss #663 on main as a false positive, matching the #630 precedent — cheapest, but it is a security-posture change;
(b) land a separate one-line PR adding a scoped CodeQL suppression beside the existing # nosemgrep, which keeps the alert reviewable in a small diff instead of buried in a 13k-line refactor.

I recommend (b). Either way it is non-blocking for this PR, but until one of them lands the alert will keep reddening that check on any future large PR that touches the config package.

[bolichen97]

Answering the one remaining First Principles point on fd62d00d4c9c — the watch note that "two doc edits ride along undeclared". The lane's single Subtraction (loader.math / loader._urlsplit) is adjudicated in its own disposition record and is unchanged; this comment covers only the undeclared-edits observation, which is correct.

The two files are docs/request-for-change/rfc-crew-agent-sdk-boundary.md and docs/system-specs/features/agent-host-contract.md. Each carried one seam-census row whose pointer at the degraded-backend log was line-anchored into the module this PR splits, spelled config/loader.py:4647-4652. This change renumbers loader.py wholesale, so that anchor would survive as a pointer at unrelated lines. Both rows now name acp_backends.resolve_selected_backend(), which is where the logger.warning actually lives (src/kiro_crew/acp_backends.py, inside that function). The edits are one table row each, documentation-only, and carry no behaviour, schema or API change — which is why they were folded into the "Updated the config owning spec" bullet rather than called out separately. Naming them explicitly in the record: agreed, and done here.

I am recording this in the conversation rather than re-opening the description on purpose: codex-review.yml fires on edited, so a body edit re-runs a currently green GPT 5.6 lane, and that is not a trade worth making for an advisory note the review itself judged "accurate (the log is at acp_backends.py:243) and forced by the move".

[chenmingwei23]

Approved: PR Readiness passed, all review lanes green.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7195 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7195: REBASE. 7304 merged at 2026-09-02T03:57:34Z and the merge conflict label was applied to 7195 twenty seconds later at 2026-09-02T03:57:54Z. This is a pure relocation conflict in one file, not a behavior conflict. Files: src/kiro_crew/config/loader.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.