Repository navigation
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe network-policy persistence, router, inheritance, and migration changes are correct; the only new findings are two optional web-form suggestions in Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (26 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 21f4cc1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 21f4cc1)Status: No Issues Found | Recommendation: Merge Executive SummaryFull re-review of the environment network-policy change (the incremental base commit is no longer an ancestor, so I reviewed the complete PR diff against Assumptions: Files Reviewed (25 files)
Previous review (commit 0fd5178)Status: No Issues Found | Recommendation: Merge Executive SummaryThe two follow-up commits are behavior-preserving: they reuse the shared Assumptions: The 56 upstream files brought in by the "Merge main" commit were not re-reviewed as PR-authored changes. Files Reviewed (2 files changed since last review)
Previous review (commit 6be97d3)Status: 4 Issues Found | Recommendation: Address before merge Executive SummaryThe main risk is that the persisted network policy becomes writable through the MCP-exposed Assumptions: Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (25 files)
Reviewed by deepseek-flash-latest · Input: 135.3K · Output: 33.4K · Cached: 3.6M Review guidance: REVIEW.md from base branch |
eshurakov
changed the title
0fd5178 to
21f4cc1
Compare
Visual verificationTested revision: Controlled demonstration; does not prove a live integration.
Controlled demonstration; does not prove a live integration.
Privacy review:
|
21f4cc1 to
2f72cdc
Compare
| saved.sandboxAllocation !== draft.sandboxAllocation | ||
| saved.sandboxAllocation !== draft.sandboxAllocation || | ||
| saved.networkAccess.mode !== draft.networkAccess.mode || | ||
| saved.networkAccess.domains !== draft.networkAccess.domains |
There was a problem hiding this comment.
[SUGGESTION]: Hidden domain text keeps the form dirty after switching back to None or Full
EnvironmentNetworkField retains domains when the mode changes, and buildEgressPolicy ignores that hidden text for None/Full, but this comparison still counts it. Switching Limited → Full (or None) therefore leaves hasEnvironmentFormChanges true even though saving sends no policy, so EnvironmentBuildPanel blocks Build/Refresh with "Save your changes before building." Compare domains only while the mode is limited.
| saved.networkAccess.domains !== draft.networkAccess.domains | |
| (saved.networkAccess.mode === 'limited' && saved.networkAccess.domains !== draft.networkAccess.domains) |
| } | ||
| ); | ||
| if (!parsed.success) { | ||
| const tooMany = parsed.error.issues.some(issue => issue.code === 'too_big'); |
There was a problem hiding this comment.
[SUGGESTION]: Overlong hostname is reported as a domain-count error
egressDomainSchema also has .max(254), which emits the same too_big code as the array limit, so a single hostname longer than 254 characters shows "Add no more than 100 domains." Restrict the check to the array-level issue (path length 1).
| const tooMany = parsed.error.issues.some(issue => issue.code === 'too_big'); | |
| const tooMany = parsed.error.issues.some(issue => issue.code === 'too_big' && issue.path.length === 1); |
Summary
Add None, Limited, and Full network access controls to repository environments. Restricted modes retain the server-owned Kilo, Git, and repository-host baseline. Limited accepts validated additional hostnames.
Persist the policy and apply it during session preparation. Preserve explicit API overrides and existing environment ownership checks. Scoped children inherit the parent's recorded policy and fail closed when it cannot be validated.
Verification
Visual Changes
Repository environment settings now include a None/Limited/Full selector and a domain editor for Limited. Required Kilo/Git access stays visible in the help text.
Reviewer Notes
These are owner-managed session defaults. Owner-level account credentials can change them; they are not an immutable limit against those credentials. Existing personal ownership and organization management-role checks apply. Scoped child credentials cannot edit environments.
Apply the additive database migration first, then the Worker, then the web changes. The generated migration retains its normal table lock; production lock duration was not measured.
Final local verification at
0fd5178900beee5cf9f6ade89d4b56c5b16eedf9: 865 selected tests, three relevant typechecks, scoped lint, formatting, and whitespace checks passed. Browser/runtime evidence above remains prior-revision evidence. No deployment or production verification occurred.Kojo published the branch, processed all four Kilo review threads, pushed the schema cleanup, and posted replies before resolving the threads. Fresh GitHub checks are tracked separately from local verification.