Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions INFRASTRUCTURE_CHANGE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Review: Infrastructure changes in PR #9978

> NOTE: this report was reconstructed from the review subagent's final summary because an earlier scratch write of the markdown was lost when the working tree was reset. The findings below match what the agent reported. A human should re-run the spot-checks for any line items they care about.

## Methodology

1. Listed all files changed by PR #9978 vs Kilo `main` (`c1ea8100e`).
2. Filtered to infrastructure-flavoured paths:
- `.github/`, `.devcontainer/`, root `script/`, `nix/`
- `flake.nix`, `flake.lock`, any `*.nix`
- `Dockerfile*`, `docker-compose*`, `packages/containers/**`
- `turbo.json`, `bunfig.toml`, `pnpm-workspace.yaml`, `vercel.json`, `netlify.toml`
- `.changeset/config.json`
- Workspace-level `package.json` files (root + per-package)
- Root `tsconfig*.json`, per-package `tsconfig*.json`
- `release-please*` configs, semantic-release configs
3. Inspected each file's diff via `git diff c1ea8100e..pr-9978 -- <file>`.

## Infra-relevant files in the diff

| File | Verdict |
|---|---|
| `.github/**` | No changes. |
| `script/` (root) | No changes. |
| `flake*`, `nix/` | No changes. |
| `packages/containers/**` (Dockerfiles) | No changes. |
| `turbo.json`, `bunfig.toml`, `.changeset/config.json`, `.devcontainer/` | No changes. |
| Root `package.json` | **Needs human review** — see Findings #1, #2, #3, #4. |
| `packages/opencode/script/build.ts` | **Needs human review** — see Findings #5. |
| `packages/opencode/package.json` | Mostly upstream version bump + dependency drift — no Kilo-specific infra concern. |

## Findings

1. **`postinstall` regression (real).** Root `package.json` drops `&& bun run script/setup-git.ts` from `postinstall`. The script still exists in-repo, and per `AGENTS.md` it's what wires up `merge.conflictStyle=zdiff3` for every contributor — i.e. the very thing that makes upstream merges like this PR tractable. **Should be restored before merging.**

2. **Dead `dev:desktop` / `dev:web` / `dev:console` scripts** added to root `package.json`. They target `packages/desktop-electron`, `packages/app`, `packages/console`, none of which exist in Kilo. Cosmetic upstream noise; safe to keep removed if a follow-up cleanup pass is convenient.

3. **`dev-setup` root script removed.** The underlying CLI still works via `./bin/kilodev dev-setup`, but the `bun run dev-setup` shorthand documented in older onboarding material is gone. Low severity — flag for docs/onboarding update only.

4. **`@sentry/solid` + `@sentry/vite-plugin`** added to the root catalog with zero consumers in Kilo's source. They're upstream's desktop integration. Safe but adds install weight; consider trimming.

5. **Dead `sourcemapsFlag` constant** in `packages/opencode/script/build.ts` (`const sourcemapsFlag = process.argv.includes("--sourcemaps")`). Kilo enforces its own sourcemap policy via a `kilocode_change`, so this flag is read but never consumed. Low severity.

## Conclusion

The PR cleanly preserves Kilo's infrastructure perimeter. There are zero changes to `.github/`, root `script/`, `flake*`, `nix/`, `packages/containers/**`, `turbo.json`, `bunfig.toml`, `.changeset/config.json`, or `.devcontainer/`. The merge resolution kept Kilo's side for all of those.

The only real regression is **Finding #1** (the lost `setup-git.ts` step). The other findings are upstream cosmetic noise.
43 changes: 43 additions & 0 deletions KILOCODE_CHANGE_MARKERS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Review: Accidental kilocode_change marker removal in PR #9978

> NOTE: this report was reconstructed from the review subagent's final summary because an earlier scratch write of the markdown was lost when the working tree was reset. The methodology, raw counts and verdicts below match what the agent reported. A human should re-run the spot-checks for any line items they care about.

## Methodology

1. Listed all files changed by PR #9978 vs Kilo `main` (`c1ea8100e`):
`git diff --name-only c1ea8100e..pr-9978`
2. Filtered out paths that are exempt from `kilocode_change` markers:
- `packages/opencode/src/kilocode/**`, `packages/opencode/test/kilocode/**`, anything else with `kilocode` in the path
- `packages/kilo-*` packages (entirely Kilo-owned)
- `packages/sdk/js/src/v2/gen/**` (auto-generated)
3. For every remaining file, computed marker counts before / after:
```
git show c1ea8100e:<file> | grep -c kilocode_change
git show pr-9978:<file> | grep -c kilocode_change
```
4. Investigated every file where the count went **down**.
5. Cross-checked: enumerated all files on `c1ea8100e` that contained `kilocode_change` and verified each is either preserved or accounted for in the PR.

## Files where marker count changed (focused list)

| File | Before | After | Verdict |
|---|---|---|---|
| `packages/opencode/src/server/middleware.ts` | 5 | 3 | OK — markers migrated to extracted `packages/opencode/src/server/cors.ts` (0→3) along with the CORS logic. |
| `packages/opencode/src/server/proxy.ts` | 2 | 1 | OK — upstream now uses `Workspace.Service.use(...)` which makes the missing-`await` bug Kilo was patching structurally impossible. Marker correctly retired. |
| `packages/opencode/src/server/routes/instance/httpapi/middleware/authorization.ts` | 2 | 1 | OK — block marker replaced by an inline marker on `Config.withDefault("kilo")`. Same Kilo behavior, terser annotation. |
| `packages/opencode/src/server/routes/ui.ts` | 6 | 3 | OK — upstream rewrote the file. Remaining 3 markers cover the proxy-fallback-replaced-with-404 behavior in both code paths. |
| `packages/opencode/test/server/httpapi-instance.test.ts` | 1 | 0 | OK — marker migrated to the new `httpapi-instance.legacy.test.ts` (0→1), which is the file that still uses the Hono bridge that the marker described. |
| `packages/opencode/src/session/system.ts` | n | n−1 | **Suspicious** — see Findings #1. |
| `packages/opencode/test/cli/tui/thread.test.ts` | n | n−5 | **Suspicious** — see Findings #2. |

All other shared files in the PR retained or grew their marker count.

## Findings

1. **`packages/opencode/src/session/system.ts` lost a standalone `// kilocode_change` marker** that sat right under the Kilo-only `(model, editorContext)` signature. The most likely annotation target was `const project = Instance.project` (legitimately gone in upstream's refactor), but the inline marker on the modified function signature deserves a sanity check. A human should confirm that the surviving `(model, editorContext)` signature still carries some form of `kilocode_change` annotation, or that no Kilo-specific behavior remains around the lost marker.

2. **`packages/opencode/test/cli/tui/thread.test.ts` had its 5 Kilo-default-args (`$0:"kilo"`, `cloud-fork`, `mdns-domain:"kilo.local"`, etc.) removed** when upstream's structural rewrite simplified the test. Production defaults still exist in `packages/opencode/src/cli/cmd/tui/thread.ts` and are still annotated with `kilocode_change`, but no test now asserts they flow through `TuiThreadCommand.handler`. Loss of marker is technically fine (the test changed), but there is now a coverage gap for the Kilo defaults. A human should decide whether to port the assertions into the new test shape.

## Conclusion

No clearly accidental marker losses were found. All decreases in marker counts are traceable to legitimate migrations (markers moved with their code), upstream catching up to Kilo (markers retired), or upstream rewrites where the marker placement no longer applies. Two items above are flagged for human verification.
62 changes: 62 additions & 0 deletions TESTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# Review: Removed Kilo-specific tests in PR #9978

> NOTE: this report was reconstructed from the review subagent's final summary because an earlier scratch write of the markdown was lost when the working tree was reset. The findings below match what the agent reported. A human should re-run the spot-checks for any line items they care about.

## Methodology

1. Listed deleted files in the PR:
`git log --diff-filter=D --name-only --pretty=format: c1ea8100e..pr-9978 | sort -u | grep -E '\.test\.(ts|tsx)$'`
2. For every test file touched by the PR (`git diff --name-only c1ea8100e..pr-9978 -- '**/*.test.*'`), diffed BEFORE vs AFTER test names:
```
git show c1ea8100e:<file> | grep -E '^\s*(it|test|describe)\(' > /tmp/before
git show pr-9978:<file> | grep -E '^\s*(it|test|describe)\(' > /tmp/after
diff /tmp/before /tmp/after
```
3. Looked for new `*.skip` / `*.todo` / `xit` markers in the diff:
```
git diff c1ea8100e..pr-9978 -- '**/*.test.*' | grep -E '^\+.*\b(skip|todo|xit)\b'
```
4. Cross-referenced with explicit skip commits (`0c1be058e`, `8cdfaea01`, plus `a1b0923a3`).

## Deleted test files

| File | Verdict |
|---|---|
| `packages/opencode/test/control-plane/sse.test.ts` | OK — upstream removed alongside its source in PR #25018. |
| `packages/opencode/test/plugin/workspace-adaptor.test.ts` | OK — renamed to `workspace-adapter.test.ts` (rename surfaces in the diff as delete+add); Kilo's `KILO_DISABLE_DEFAULT_PLUGINS` / `Flag.KILO_EXPERIMENTAL_WORKSPACES` setup was preserved. |
| `packages/opencode/test/control-plane/adaptors.test.ts` | OK — replaced by `adapters.test.ts` (the rename Kilo source already adopted). |
| `packages/opencode/test/kilocode/session-message-metadata.test.ts` | **Suspicious** — see Findings #4. |

No other Kilo-only test files were deleted.

## Kilo tests removed from shared files (all properly ported)

| Source file | Migration |
|---|---|
| `test/session/instruction.test.ts` | `KILO_CONFIG_DIR` trio ported into `it.live` form inside `Instruction.systemPaths global config`, with `kilocode_change` marker preserved. |
| `test/server/httpapi-instance.test.ts` | Four Kilo Hono-bridge tests moved into the new `test/server/httpapi-instance.legacy.test.ts`. |
| `test/config/agent-color.test.ts` | Kilo-marker assertions (`code` agent rename, `#FFA500` color) preserved in the structural rewrite. |
| `test/server/httpapi-workspace.test.ts` | Kilo tests ported to `it.live`; three new tests added. The dropped `test.todo("proxies remote workspace websocket through real Effect listener")` is now covered by `httpapi-workspace-routing.test.ts`. |
| `explore agent asks for external directories…` | Renamed and tied to the `external_directory` whitelist fix commit. |

## Newly-skipped suites

Three `describe.skip`s introduced — all skip upstream-added tests, not pre-existing Kilo tests:

1. **`Workspace.sessionRestore`** — entire suite skipped via commit `0c1be058e`. "Tracked for follow-up" in the commit message but no ticket. Suite contains some Kilo-flavored coverage. **Medium severity.**
2. **`HttpApi JSON parity`** — skipped (`8cdfaea01`); upstream parity test incompatible with Kilo's `NullOr` schemas. **Low severity.**
3. **`ModelsDev Service`** — skipped; Kilo's provider filtering has no replacement Kilo-side test. **Low severity.**

One test was intentionally rewritten in `instance.test.ts` (commit `a1b0923a3`) to match Kilo's contract; properly marked with `kilocode_change`.

## Findings

1. **`Workspace.sessionRestore` whole suite is silently `describe.skip`'d** with no ticket linked. Some of its assertions covered Kilo-flavored behavior (workspace adapter restore). A human should decide whether to file a follow-up to re-enable or replace it.
2. **`ModelsDev Service` skip** leaves Kilo's provider-filtering coverage at zero. Consider adding a Kilo-side replacement test.
3. **`HttpApi JSON parity` skip** is justified by the `NullOr` schema diff but should be revisited if/when schemas re-converge.
4. **`packages/opencode/test/kilocode/session-message-metadata.test.ts` was deleted.** This file lives in `test/kilocode/` and is therefore Kilo-only. The agent did not surface a clear migration target. A human should confirm: (a) was the production code it covered also removed, (b) was the coverage moved to another `test/kilocode/` file, or (c) is this an accidental deletion?
5. The `proxies remote workspace websocket through real Effect listener` `test.todo` was dropped without leaving a placeholder. Coverage is asserted to live in `httpapi-workspace-routing.test.ts` — verify this is true end-to-end for the Kilo-specific remote-workspace path.

## Conclusion

No Kilo test file was clearly accidentally deleted as part of merge auto-resolution. The Kilo tests removed from shared files were all ported. The biggest follow-up concerns are the silently-skipped suites (especially `Workspace.sessionRestore`) and the one Kilo-only deletion flagged in Finding #4.
36 changes: 36 additions & 0 deletions UNNECESSARY_MARKERS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Review: Stale `kilocode_change` markers after PR #9978

> NOTE: this report was reconstructed from the review subagent's final summary because an earlier scratch write of the markdown was lost when the working tree was reset. The findings below match what the agent reported. A human should re-run `bun run script/upstream/find-reset-candidates.ts --dry-run packages/opencode` themselves to reproduce.

## Methodology

1. Ran the canonical tool, scoped to `packages/opencode`, against the PR branch:
```
bun run script/upstream/find-reset-candidates.ts --dry-run --concurrency 16 packages/opencode
```
(Compared to last merged upstream snapshot `006a05abe` / v1.14.33.)
2. Intersected each bucket with the 254 files changed by PR #9978.
3. For each `markers-only` / `identical` candidate inside the PR diff, verified by spot-checking with:
```
bun run script/upstream/reset-to-upstream.ts --dry-run <file>
```
4. Manually confirmed the merge-resolution flashpoints called out in the PR description (`src/server/proxy.ts`, `src/server/routes/global.ts`) by comparing against transformed upstream.

## Bucket summary (PR-touched ∩ candidates)

| Bucket | Count | PR-touched? | Notes |
|---|---|---|---|
| `markers-only` | 0 | n/a | **Empty** — no stale-markers files to reset. |
| `identical` | 32 | yes | Spot-checked sample contains zero markers; differs from raw upstream only via `translate()` branding/package-name transforms. |
| `whitespace-only` | 0 | n/a | None. |
| `small-diff` | 23 | yes | Each carries markers around real Kilo additive code (e.g. `src/server/cors.ts`, `src/server/server.ts`, `src/storage/db.ts`); confirmed via `git diff 006a05abe..pr-9978 -- <file>`. |
| `large-diff` | 72 | yes | Substantive Kilo-specific changes including `src/server/proxy.ts` and `src/server/routes/global.ts`; markers guard real logic. |
| `cosmetic-only` | 1 | **no** | `packages/opencode/src/session/prompt/anthropic.txt` — single trailing-space drift, not touched by this PR; pre-existing finding. |

## Findings

None. No `markers-only` candidates were detected in the PR-touched file set; no resets are recommended for this PR.

## Conclusion

PR #9978 does not introduce any newly-stale `kilocode_change` markers. All markers in PR-touched files still guard real Kilo behavior. The cosmetic-only `anthropic.txt` finding is independent of this PR.
Loading