feat: [de-fork S1] baseline metrics + upstream-merge replay tooling#1013
feat: [de-fork S1] baseline metrics + upstream-merge replay tooling#1013anandgupta42 wants to merge 5 commits into
Conversation
Stage S1 of the de-fork spike (docs/internal/2026-07-18-defork-spike-spec.md).
Measurement foundation for shrinking the fork's upstream-shared footprint —
zero product-behavior change; all new code is fork-owned tooling in
`script/upstream/`.
Tools (share one taxonomy module):
- `taxonomy.ts` — 3-bucket classification (`upstream_shared` / `fork_owned` /
`fork_added_outside_boundary`) against the pinned `v1.17.9` upstream tree,
upstream-existence precedence (10 `.opencode/` overlap paths pinned by a
generated oracle test).
- `census.ts` — stack-based `altimate_change` block parser reading from the git
tree (not the working FS), versioned provenance-pinned JSON envelope, and a
multiset `{file,contentHash}` ratchet (`--check`) with quantity-scoped
committed exemptions.
- `divergence.ts` — files/hunks/±lines vs the upstream base via
`git diff --numstat` (git-exact; matches `git --shortstat`). Diff config is
pinned (`diff.algorithm=myers`, `interHunkContext=0`, `core.quotepath=false`,
`--no-ext-diff --no-textconv`, `GIT_DIFF_OPTS` stripped) so numbers reproduce
across git environments. Baseline: 5283 files, +499181/-740989.
- `replay.ts` — explicit-base three-way merge via
`git merge-tree --write-tree --merge-base` (object-db only, never mutates the
working tree). Reports honest conflict surface: 651 conflicted paths, 118
content files / 466 textual regions, 4 binary conflicts, and 94 truly-clean
auto-merges (not the 212 attempts). Fail-closed region/binary reads; requires
git >= 2.40.
Includes committed baselines, an empty exemptions file, a JSONC parser, and the
unclosed-marker allowlist. `refs.ts` runs all git via arg-vector `spawnSync`
(no shell interpolation).
Test Plan:
- `bun test` in `script/upstream`: 155 pass, 0 fail (nesting, unmatched-marker
handling, taxonomy overlap oracle, multiset ratchet pass+fail, envelope
determinism, numstat `++`/`--` content regression, merge-tree parsing against
a real fixture repo, honest replay metrics).
- Divergence pinned against `git --shortstat` ground truth; verified
config-immune (yields the oracle even under `diff.algorithm=patience`).
- Ratchet verified: passes at baseline, fails on a synthetic net-new marker.
- `bunx tsc --noEmit -p script/upstream`: clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
This PR has 97,130 reviewable changed lines after ignored/generated files are excluded, above cubic's default 50,000-changed-line automatic review limit. Most of the diff comes from:
Comment |
📝 WalkthroughWalkthroughThis change adds local Git and JSONC utilities, taxonomy classification, deterministic census and divergence reports, merge replay analysis, comprehensive Bun tests, configuration allowlists, CI wiring, and generated upstream baseline artifacts. ChangesUpstream de-fork tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReplayCLI
participant GitMergeTree
participant GitObjectDatabase
participant CensusEnvelope
ReplayCLI->>GitMergeTree: Run explicit-base replay
GitMergeTree-->>ReplayCLI: Return tree and conflict records
ReplayCLI->>GitObjectDatabase: Read conflict stage blobs
GitObjectDatabase-->>ReplayCLI: Return blob data
ReplayCLI->>CensusEnvelope: Attribute conflict paths
CensusEnvelope-->>ReplayCLI: Return bucket counts
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
script/upstream/replay.ts (1)
453-467: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize census block lookup to O(1).
Using
.find()within the loop results in an O(N × M) operation. You can simplify the code, improve performance, and safely drop the non-null assertion by building aMapinstead of aSet.♻️ Proposed refactor
- const censusFilesWithBlocks = new Set(census.blocks.map((b) => b.file)) + const censusBucketByFile = new Map(census.blocks.map((b) => [b.file, b.bucket])) const counts = new Map<Bucket, number>() // Authoritative conflicted-path set = the stage-line paths (one entry per // conflicted path). Do NOT union in conflictMessage paths: a rename message // names BOTH the old and new path, which double-counts (that was the // 750-vs-651 discrepancy). The stage lines are the ground truth for "which // paths conflicted." const conflictedFilePaths = new Set(envelope.conflictedPaths.map((p) => p.path)) for (const path of conflictedFilePaths) { // A conflicted path only gets attributed if it's a file the census // actually scanned marker blocks in; otherwise fall back to plain // taxonomy classification against the upstream tree. - const bucket = censusFilesWithBlocks.has(path) ? census.blocks.find((b) => b.file === path)!.bucket : classifyBucket(path, upstreamPaths) + const bucket = censusBucketByFile.get(path) ?? classifyBucket(path, upstreamPaths) counts.set(bucket, (counts.get(bucket) ?? 0) + 1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/upstream/replay.ts` around lines 453 - 467, Replace censusFilesWithBlocks and the per-path census.blocks.find lookup in the conflictedFilePaths loop with a Map keyed by block.file and containing each block’s bucket. Use the mapped bucket when available, otherwise retain classifyBucket(path, upstreamPaths), eliminating the non-null assertion while preserving the existing counting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/upstream/divergence.ts`:
- Around line 157-181: Update parseNumstatZ to fail closed on malformed input:
throw when a record head lacks either tab delimiter, throw when a rename/copy
record is missing either old- or new-path token instead of defaulting to empty
strings, and validate parsed added/deleted counts so NaN values are rejected
before records are emitted. Match the sibling census.ts parseNumstatZ
error-handling behavior while preserving valid binary, rename, and regular
records.
In `@script/upstream/replay.ts`:
- Around line 174-175: Update runMergeTree’s git merge-tree invocation to pass
Git’s -c core.quotepath=false configuration before the merge-tree subcommand,
ensuring emitted paths remain raw and consistent with ls-tree -z consumers.
In `@script/upstream/taxonomy.test.ts`:
- Around line 53-101: Refactor the overlap test in the “.opencode/ overlap
oracle” describe block to reuse the scoped path-listing helper from
utils/refs.ts instead of issuing inline git ls-tree and git ls-files execSync
calls. Ensure the helper uses the existing consistent reference/error handling
and supports listing .opencode/ paths for UPSTREAM_TAG and the current tree,
while preserving the overlap and classification assertions.
- Around line 1-15: Replace the inline execSync implementation in
loadUpstreamPaths with the shared utils/refs.ts loadPathsAtRef flow so ref
resolution goes through resolveRefOrThrow and preserves its actionable
missing-ref error. Ensure the test setup or CI fetches the v1.17.9 tag before
these tests execute, while keeping the existing Set<string> result expected by
the taxonomy tests.
In `@script/upstream/utils/refs.ts`:
- Around line 129-166: Update readBlobsBatch to deduplicate oids before
constructing the git cat-file --batch input, so git emits exactly one block per
processed OID. Iterate over the deduplicated list while parsing and remove the
map.has skip logic, preserving one returned content entry per unique OID and
keeping input-order processing intact.
---
Nitpick comments:
In `@script/upstream/replay.ts`:
- Around line 453-467: Replace censusFilesWithBlocks and the per-path
census.blocks.find lookup in the conflictedFilePaths loop with a Map keyed by
block.file and containing each block’s bucket. Use the mapped bucket when
available, otherwise retain classifyBucket(path, upstreamPaths), eliminating the
non-null assertion while preserving the existing counting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2dc88003-4cb9-4f7b-a1a0-233545efeac4
📒 Files selected for processing (18)
script/upstream/baselines/2026-07-18/census.jsonscript/upstream/baselines/2026-07-18/divergence-v1.17.9.jsonscript/upstream/baselines/2026-07-18/replay-v1.18.3.jsonscript/upstream/baselines/README.mdscript/upstream/census.test.tsscript/upstream/census.tsscript/upstream/defork-exemptions.jsoncscript/upstream/divergence.test.tsscript/upstream/divergence.tsscript/upstream/replay.test.tsscript/upstream/replay.tsscript/upstream/taxonomy.test.tsscript/upstream/taxonomy.tsscript/upstream/unclosed-marker-allowlist.jsoncscript/upstream/utils/jsonc.test.tsscript/upstream/utils/jsonc.tsscript/upstream/utils/refs.tsscript/upstream/utils/repo-root.ts
| import { describe, test, expect } from "bun:test" | ||
| import { execSync } from "child_process" | ||
| import { classifyBucket, classifyCategories, FORK_OWNED_ROOTS, UNATTRIBUTED, categoryRuleIds, isUpstreamFixLine } from "./taxonomy" | ||
|
|
||
| const REPO_ROOT = execSync("git rev-parse --show-toplevel", { encoding: "utf-8" }).trim() | ||
| const UPSTREAM_TAG = "v1.17.9" | ||
|
|
||
| function loadUpstreamPaths(): Set<string> { | ||
| const out = execSync(`git ls-tree -r --name-only "${UPSTREAM_TAG}"`, { | ||
| cwd: REPO_ROOT, | ||
| encoding: "utf-8", | ||
| maxBuffer: 50 * 1024 * 1024, | ||
| }) | ||
| return new Set(out.split("\n").filter((l) => l.length > 0)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Test setup duplicates refs.ts ref-resolution logic, and depends on a local tag that CI doesn't have.
loadUpstreamPaths() here re-implements utils/refs.ts#loadPathsAtRef via inline execSync, bypassing resolveRefOrThrow's designed behavior of failing with an actionable git fetch upstream tag v1.17.9 --no-tags message when the ref is missing locally. Per the pipeline failure logs, this is exactly what's happening in CI: git ls-tree -r --name-only "v1.17.9" -- .opencode/ fails with fatal: Not a valid object name v1.17.9 (surfaced at Line 55), producing a raw, unhelpful execSync crash instead of the friendly fetch instructions the rest of the toolset provides.
Reusing loadPathsAtRef/resolveRefOrThrow here would both remove the duplication and turn this CI failure into an actionable error; separately, the CI job (or a beforeAll) needs to ensure v1.17.9 is fetched before these tests run.
♻️ Suggested consolidation
-import { execSync } from "child_process"
+import { loadPathsAtRef } from "./utils/refs"
+import { resolveRepoRoot } from "./utils/repo-root"
import { classifyBucket, classifyCategories, FORK_OWNED_ROOTS, UNATTRIBUTED, categoryRuleIds, isUpstreamFixLine } from "./taxonomy"
-const REPO_ROOT = execSync("git rev-parse --show-toplevel", { encoding: "utf-8" }).trim()
+const REPO_ROOT = resolveRepoRoot()
const UPSTREAM_TAG = "v1.17.9"
function loadUpstreamPaths(): Set<string> {
- const out = execSync(`git ls-tree -r --name-only "${UPSTREAM_TAG}"`, {
- cwd: REPO_ROOT,
- encoding: "utf-8",
- maxBuffer: 50 * 1024 * 1024,
- })
- return new Set(out.split("\n").filter((l) => l.length > 0))
+ return loadPathsAtRef(UPSTREAM_TAG, REPO_ROOT)
}🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync } from "child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/upstream/taxonomy.test.ts` around lines 1 - 15, Replace the inline
execSync implementation in loadUpstreamPaths with the shared utils/refs.ts
loadPathsAtRef flow so ref resolution goes through resolveRefOrThrow and
preserves its actionable missing-ref error. Ensure the test setup or CI fetches
the v1.17.9 tag before these tests execute, while keeping the existing
Set<string> result expected by the taxonomy tests.
Source: Pipeline failures
| describe(".opencode/ overlap oracle (generated from git ls-tree -r v1.17.9)", () => { | ||
| test("exactly 10 .opencode/ paths overlap between v1.17.9 and the current tree, and all classify upstream_shared", () => { | ||
| const upstreamOpencodePaths = execSync(`git ls-tree -r --name-only "${UPSTREAM_TAG}" -- .opencode/`, { | ||
| cwd: REPO_ROOT, | ||
| encoding: "utf-8", | ||
| }) | ||
| .split("\n") | ||
| .filter((l) => l.length > 0) | ||
|
|
||
| const currentOpencodePaths = execSync("git ls-files .opencode/", { | ||
| cwd: REPO_ROOT, | ||
| encoding: "utf-8", | ||
| }) | ||
| .split("\n") | ||
| .filter((l) => l.length > 0) | ||
|
|
||
| const currentSet = new Set(currentOpencodePaths) | ||
| const overlap = upstreamOpencodePaths.filter((p) => currentSet.has(p)).sort() | ||
|
|
||
| expect(overlap).toHaveLength(10) | ||
| expect(overlap).toEqual( | ||
| [ | ||
| ".opencode/.gitignore", | ||
| ".opencode/command/commit.md", | ||
| ".opencode/command/issues.md", | ||
| ".opencode/command/learn.md", | ||
| ".opencode/command/translate.md", | ||
| ".opencode/opencode.jsonc", | ||
| ".opencode/plugins/smoke-theme.json", | ||
| ".opencode/plugins/tui-smoke.tsx", | ||
| ".opencode/themes/.gitignore", | ||
| ".opencode/tui.json", | ||
| ].sort(), | ||
| ) | ||
|
|
||
| const upstreamPaths = new Set(overlap) | ||
| expect(classifyBucket(".opencode/.gitignore", upstreamPaths)).toBe("upstream_shared") | ||
| expect(classifyBucket(".opencode/themes/.gitignore", upstreamPaths)).toBe("upstream_shared") | ||
| for (const p of overlap) { | ||
| expect(classifyBucket(p, upstreamPaths)).toBe("upstream_shared") | ||
| } | ||
| }) | ||
|
|
||
| test("a .opencode/ path NOT in the overlap set classifies fork_owned", () => { | ||
| const upstreamPaths = loadUpstreamPaths() | ||
| // .opencode/skills/** is fork-authored and has no upstream counterpart. | ||
| expect(classifyBucket(".opencode/skills/dbt-snapshot/SKILL.md", upstreamPaths)).toBe("fork_owned") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Same missing-tag dependency as above; also duplicates git-invocation logic instead of using utils/refs.ts.
This describe block runs two more inline git ls-tree/git ls-files execSync calls against UPSTREAM_TAG, hitting the same CI failure mode (missing local v1.17.9) confirmed by the pipeline logs. Consider factoring this scoped .opencode/-only listing through utils/refs.ts (e.g. a git ls-tree -r -z --name-only <ref> -- <path>-based helper) for consistent error handling, once the setup fix above is applied.
🧰 Tools
🪛 GitHub Actions: CI / 7_Marker Guard.txt
[error] 55-55: Test failed due to Git command error: git ls-tree -r --name-only "v1.17.9" -- .opencode/ returned fatal: Not a valid object name v1.17.9 (Command failed in execSync).
🪛 GitHub Actions: CI / Marker Guard
[error] 55-55: Test failed when executing command git ls-tree -r --name-only "v1.17.9" -- .opencode/. git reported: "fatal: Not a valid object name v1.17.9" leading to Command failed: ... in execSync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/upstream/taxonomy.test.ts` around lines 53 - 101, Refactor the overlap
test in the “.opencode/ overlap oracle” describe block to reuse the scoped
path-listing helper from utils/refs.ts instead of issuing inline git ls-tree and
git ls-files execSync calls. Ensure the helper uses the existing consistent
reference/error handling and supports listing .opencode/ paths for UPSTREAM_TAG
and the current tree, while preserving the overlap and classification
assertions.
Source: Pipeline failures
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cf1b67b62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| This is also the **ratchet baseline** wired into CI (see | ||
| `.github/workflows/ci.yml`'s `marker-guard` job): `census.ts --check |
There was a problem hiding this comment.
The baseline is documented as an active CI ratchet, but the inspected .github/workflows/ci.yml marker-guard job at lines 486-535 only runs the tests and analyze.ts --markers; it never invokes census.ts --check --baseline. Consequently, new marker blocks covered only by this multiset ratchet can merge while CI remains green, despite this documentation telling maintainers that they are blocked.
Useful? React with 👍 / 👎.
| function runMergeTree(repoRoot: string, baseSha: string, oursSha: string, targetSha: string): { stdout: string; exitCode: number } { | ||
| const result = spawnSync("git", ["merge-tree", "--write-tree", "--merge-base", baseSha, oursSha, targetSha], { |
There was a problem hiding this comment.
When a runner has repository or global merge.renames or merge.renameLimit settings, this invocation inherits them, so rename/delete and file-location detection—and therefore the committed conflict counts—can differ for identical refs. The inspected git help --config explicitly lists merge.renames, merge.renameLimit, and merge.directoryRenames as merge configuration; pass explicit -c values as divergence.ts does for its diff settings so replay results are reproducible across environments.
Useful? React with 👍 / 👎.
| export function attributeConflictsToCensus(envelope: ReplayEnvelope, census: CensusLike, upstreamPaths: ReadonlySet<string>): ConflictCategoryAttribution[] { | ||
| if (census.oursSha !== envelope.oursSha) { |
There was a problem hiding this comment.
If --census was generated with the same oursSha but a different --upstream-base, this check accepts it even though every stored block bucket was classified against a different upstream path set. Line 465 then trusts those stale buckets for marker-bearing conflict paths while using the replay's current upstream set for other paths, producing internally inconsistent byBucket results; validate the census upstream-base SHA and taxonomy version in addition to oursSha.
Useful? React with 👍 / 👎.
| 1146 `upstream_shared` blocks across 244 files, 242 `fork_owned` blocks | ||
| across 89 files, 235 `fork_added_outside_boundary` blocks across 103 files. |
There was a problem hiding this comment.
These reported census results disagree with the adjacent committed census.json: its totals are 228 fork-owned blocks across 84 files and 236 outside-boundary blocks across 104 files, not 242/89 and 235/103. Anyone using this headline for de-fork planning or validating a regeneration receives different baseline metrics from the machine-readable source of truth.
Useful? React with 👍 / 👎.
| "v1.17.9→v1.18.3 replay reports honest regions + clean auto-merges", | ||
| () => { | ||
| const { resolveRepoRoot } = require("./utils/repo-root") | ||
| const repoRoot = resolveRepoRoot() | ||
| const env = buildReplay(repoRoot, "v1.17.9", "8a50ec7f55", "v1.18.3", { now: "2026-07-18T00:00:00.000Z" }) |
There was a problem hiding this comment.
This repository-level test requires a locally resolvable upstream v1.18.3, but the inspected marker-guard setup in .github/workflows/ci.yml:499-503 fetches the upstream remote with --no-tags. On a clean CI checkout without that upstream tag—which the new baseline README itself identifies as a supported state—buildReplay throws during ref resolution before reaching any assertion, causing the entire marker-guard test step to fail; fetch the exact tag or make the fixture self-contained.
Useful? React with 👍 / 👎.
| // Unquoted: split on " b/" — the last occurrence separates a/ from b/. | ||
| const idx = rest.lastIndexOf(" b/") | ||
| if (idx !== -1) return rest.slice(idx + 3) |
There was a problem hiding this comment.
For a legal path containing b/, such as docs/a b/file.md, Git emits an unquoted header like diff --git a/docs/a b/file.md b/docs/a b/file.md even with the pinned core.quotepath=false. lastIndexOf(" b/") selects the occurrence inside the post-image path and returns only file.md, so the subsequent hunk-attribution consistency check throws instead of producing a divergence report; derive the path using the authoritative numstat entries or another unambiguous representation.
Useful? React with 👍 / 👎.
| const ours = resolveRefOrThrow(oursRef, repoRoot) | ||
| const upstreamBase = resolveRefOrThrow(upstreamBaseRef, repoRoot) | ||
| const upstreamPaths = loadPathsAtRef(upstreamBaseRef, repoRoot) | ||
| const filesAtRef = scanFilesAtRef(oursRef, repoRoot) |
There was a problem hiding this comment.
When oursRef or upstreamBaseRef is a movable ref and another process updates it between resolution and these reads, the envelope records the earlier oursSha/tree and upstream SHA/tree but scans and classifies against the later ref contents. That silently violates the provenance-pinned contract and can create a baseline that cannot be reproduced from its recorded SHAs; pass ours.sha and upstreamBase.sha to the object-database reads after resolving them.
Useful? React with 👍 / 👎.
| export function loadUnclosedAllowlist(filePath: string): UnclosedMarkerAllowlistEntry[] { | ||
| if (!fs.existsSync(filePath)) return [] | ||
| const raw = fs.readFileSync(filePath, "utf-8") | ||
| const parsed = parseJsonc(raw) | ||
| if (!Array.isArray(parsed)) throw new Error(`${filePath}: expected top-level JSON array`) | ||
| return parsed as UnclosedMarkerAllowlistEntry[] |
There was a problem hiding this comment.
Unlike the exemption loader below, this loader casts arbitrary parsed objects directly to the allowlist type. An entry with the right file and startLine but no reason or approvedBy still suppresses a real unclosed marker, and a mistyped expires value becomes NaN, making the comparison at line 259 false and granting permanent suppression. Validate every field and reject invalid dates before the entries can bypass the census failure.
Useful? React with 👍 / 👎.
| function isExpired(exemption: Exemption, now: Date): boolean { | ||
| if (!exemption.expires) return false | ||
| return new Date(exemption.expires).getTime() < now.getTime() |
There was a problem hiding this comment.
For the documented YYYY-MM-DD form, JavaScript parses the value at midnight UTC, so an exemption with expires: "2026-12-31" is already considered expired for almost the entirety of December 31. This contradicts the interface's “after this date” contract and can make the ratchet unexpectedly fail during the stated final valid day; compare date-only values at the end of that day or document and enforce an instant-bearing timestamp.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }) | ||
| continue | ||
| } | ||
| if (END_RE.test(line) && stack.length > 0) { |
There was a problem hiding this comment.
WARNING: Orphan altimate_change end markers are silently dropped.
The end-marker branch is guarded by && stack.length > 0, so an altimate_change end with no matching open block on the stack is ignored with no error and no count. This is asymmetric with the unclosed-start case, which assembleCensus fails closed on at census.ts:344. A stray end (e.g. an accidentally-deleted start whose end now dangles, or a copy-paste error) is masked silently, which can hide the very class of marker-drift bug this tool exists to catch. Either throw on a stack-underflow end (mirroring the unclosed-start contract) or document+test the intentional tolerance.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const raw = fs.readFileSync(filePath, "utf-8") | ||
| const parsed = parseJsonc(raw) | ||
| if (!Array.isArray(parsed)) throw new Error(`${filePath}: expected top-level JSON array`) | ||
| return parsed as UnclosedMarkerAllowlistEntry[] |
There was a problem hiding this comment.
WARNING: loadUnclosedAllowlist skips the per-entry shape validation that loadExemptions enforces.
loadExemptions runs every entry through validateExemption (rejecting malformed expires, non-integer startLine, etc.), but loadUnclosedAllowlist returns parsed as UnclosedMarkerAllowlistEntry[] with no checks. Concrete impact: isAllowlistedUnclosed at census.ts:259 does new Date(e.expires).getTime() < now.getTime(); if expires is a non-date string, getTime() returns NaN, the comparison is always false, so a typo silently makes the entry never expire (a permanent suppression of the unclosed throw). A malformed startLine similarly never matches. Add a validateUnclosedAllowlistEntry mirroring validateExemption and apply it per entry.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // - --no-color / --no-ext-diff / --no-textconv: no ANSI in output, and do | ||
| // NOT let git launch external diff/textconv programs (a read-only report | ||
| // must not execute configured helpers). | ||
| const PINNED_DIFF_CONFIG = [ |
There was a problem hiding this comment.
WARNING: PINNED_DIFF_CONFIG doesn't pin diff.indentHeuristic (or the diff.ignore* family), undermining the "config-immunity" reproducibility claim.
The header comment (divergence.ts:40-43) and the PR description both promise byte-for-byte reproduction "regardless of the runner's git config," but diff.indentHeuristic (a hunk-boundary tie-breaker that runs on top of diff.algorithm=myers) is left inherited — a runner with diff.indentHeuristic=true produces a different headline hunk count (the PR's "21162 hunks" metric). Likewise diff.ignoreAllSpace/ignoreSpaceChange/ignoreSpaceAtEol/ignoreBlankLines default off but, if inherited as on, silently change the +/- counts. Add -c diff.indentHeuristic=false (and the four diff.ignore*=false) to PINNED_DIFF_CONFIG.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * discard stdout on a non-zero exit. | ||
| */ | ||
| function runMergeTree(repoRoot: string, baseSha: string, oursSha: string, targetSha: string): { stdout: string; exitCode: number } { | ||
| const result = spawnSync("git", ["merge-tree", "--write-tree", "--merge-base", baseSha, oursSha, targetSha], { |
There was a problem hiding this comment.
WARNING: runMergeTree doesn't pass -c core.quotepath=false (or -z), so conflicted paths with non-ASCII or special bytes get C-string-quoted in git's output.
CONFLICT_PATH_LINE_RE at replay.ts:72 then captures the quoted form (e.g. "\347\211\214.png"), but census paths come from git ls-tree -r -z (utils/refs.ts:81) which are raw/unquoted — so attributeConflictsToCensus's lookup censusFilesWithBlocks.has(path) at replay.ts:465 silently misses and falls back to classifyBucket, producing wrong bucket counts with no thrown error. The sibling divergence.ts:60 pins core.quotepath=false for exactly this reason; replay.ts is the asymmetric outlier. Add -c core.quotepath=false (or parse -z output).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| maxBuffer: 200 * 1024 * 1024, | ||
| // Pin locale so CONFLICT/Auto-merging message text (which we parse) is | ||
| // stable across environments. | ||
| env: { ...process.env, LC_ALL: "C" }, |
There was a problem hiding this comment.
WARNING: merge-tree env inherits GIT_DIFF_OPTS / GIT_EXTERNAL_DIFF / GIT_CONFIG_*; only LC_ALL is pinned.
All three spawnSync calls in replay.ts (runMergeTree:181, countConflictRegions:224, countBinaryConflictPaths:396) use env: { ...process.env, LC_ALL: "C" } and spread the entire inherited environment. The sibling divergence.ts:82-87 defines sanitizedGitEnv() that additionally deletes GIT_DIFF_OPTS and GIT_EXTERNAL_DIFF precisely so inherited overrides can't perturb the parsed output. For replay, an inherited GIT_EXTERNAL_DIFF or GIT_CONFIG_PARAMETERS could change CONFLICT/Auto-merging message text or path resolution on a different runner, breaking the pinned test at replay.test.ts:310-318. Reuse/extract sanitizedGitEnv() and apply it to all three callsites.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // ── CLI ────────────────────────────────────────────────────────────────── | ||
|
|
||
| function parseArgs(argv: string[]) { | ||
| const args = { ...Object.fromEntries(argv.map((a, i) => [a, argv[i + 1]])) } as Record<string, string> |
There was a problem hiding this comment.
SUGGESTION: args is built and immediately discarded (void args); it's dead code.
The flag and opt closures do all the real parsing by rescanning argv directly. The args object is never returned or read — void args exists only to silence the unused-variable lint. It's also a latent foot-gun: Object.fromEntries(argv.map((a, i) => [a, argv[i + 1]])) silently overwrites duplicate flag keys, which could mislead a future editor into thinking args is the source of truth. Delete both the args and void args lines.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| expect(envelope.upstreamBaseTree).toMatch(/^[0-9a-f]{40}$/) | ||
| expect(envelope.oursTree).toMatch(/^[0-9a-f]{40}$/) | ||
| expect(envelope.diffOptions.renameLimit).toBe(20000) | ||
| expect(envelope.files.length).toBe(envelope.totals.files) |
There was a problem hiding this comment.
SUGGESTION: This assertion is tautological and can never fail.
envelope.totals.files is set to files.length at divergence.ts:343, so this reduces to files.length === files.length. Replace with a meaningful invariant — e.g. that every entry in envelope.files has a unique path, or that binaryFiles + renameFiles + testFiles reconciles against files.filter(...).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| export function classifyBucket(relPath: string, upstreamPaths: ReadonlySet<string>): Bucket { | ||
| const normalized = normalizePath(relPath) | ||
| if (upstreamPaths.has(normalized)) return "upstream_shared" | ||
| if (FORK_OWNED_ROOTS.some((pattern) => minimatch(normalized, pattern))) return "fork_owned" |
There was a problem hiding this comment.
SUGGESTION: minimatch recompiles all 14 glob patterns on every call.
classifyBucket is invoked once per file by census.ts:145, census.ts:777, divergence.ts:294, and replay.ts:465 over a multi-thousand-file corpus, so the 14 patterns are recompiled ~14 × N times per run. Hoist const COMPILED = FORK_OWNED_ROOTS.map((p) => new minimatch.Minimatch(p)) to module scope and test COMPILED.some((m) => m.match(normalized)).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }) | ||
| }) | ||
|
|
||
| describe("buildDivergence (real-repo, pinned against git --shortstat ground truth)", () => { |
There was a problem hiding this comment.
SUGGESTION: The PR's "config-immunity" claim is not backed by a test in this block.
The PR description asserts "config-immunity verified — yields the same numbers even under diff.algorithm=patience", but this describe only contains a git --shortstat ground-truth test and a same-now determinism test — neither injects a diff config (diff.algorithm=patience, core.quotepath=true, diff.indentHeuristic=true, GIT_DIFF_OPTS=--unified=50) and re-runs. Combined with the unpinned knobs flagged at divergence.ts:52, a future regression that drops a -c flag would pass the entire suite. Add a test that injects an alternate config via GIT_CONFIG_PARAMETERS and asserts byte-identical envelopes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| expect(env.totals.autoMergedAttempted).toBe(212) | ||
| expect(env.totals.autoMergedClean).toBe(94) | ||
| // 4 binary conflicts (2 PNG, 1 TTF, 1 WOFF2) otherwise hidden under modify/delete. | ||
| expect(env.totals.binaryConflictPaths).toBe(4) |
There was a problem hiding this comment.
SUGGESTION: The fail-closed binary-read paths in countBinaryConflictPaths are not exercised.
countBinaryConflictPaths has two explicit fail-closed throws (replay.ts:407 "truncated cat-file output" and :411 "bad cat-file header" — the latter fires on a missing OID, since --batch prints <oid> missing and exits 0). Neither is covered by a constructed fixture; the pinned test here only asserts binaryConflictPaths === 4 against the real repo, so a regression that silently returns 0 (e.g. a future edit that swallows the header-mismatch) wouldn't be caught. The buildFixtureRepo helper creates no binary blob. Add a fixture that commits a PNG/TTF blob with a content or modify/delete conflict and asserts binaryConflictPaths >= 1.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Incremental pass over commit Resolved this round:
Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed this pass (4 files changed, 1 new issue)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 167c463)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 167c463)Status: 4 Issues Found | Recommendation: Address before merge Incremental pass over commit
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed this pass (8 files changed, 0 new issues)
Fix these issues in Kilo Cloud Previous review (commit 8dfb358)Status: 11 Issues Found | Recommendation: Address before merge Incremental pass over commit Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed this pass (3 files changed, 0 new issues)
Fix these issues in Kilo Cloud Previous review (commit 9f96122)Status: 11 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit 5cf1b67)Status: 10 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 code/config files; 3 JSON baselines skimmed as data)
Notes
Reviewed by glm-5.2 · Input: 63.5K · Output: 13.7K · Cached: 530.8K Review guidance: REVIEW.md from base branch |
- `git fetch upstream tag v1.17.9` so the de-fork tooling can resolve the pinned upstream base ref in CI (the branch fetch above excludes tags). - Adds `test/altimate/trace-golden/` to the dedicated subprocess test pass so the trace-golden live E2E oracle actually runs in CI (it's otherwise skipped by OPENCODE_SKIP_SUBPROCESS=1 in the main pass). The census-ratchet CI step is deferred to a follow-up: census reads markers from the git tree, and now that script/upstream/ is committed it scans its own marker-checking tooling (analyze.ts, census.ts, ...) which legitimately contain marker syntax as code. The ratchet needs census.ts to exclude its own tooling directory from the scan first; tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
| # exit code. --timeout 90000 gives headroom for slow fs/spawn tests.) | ||
| SUBPROCESS_PATHS="test/cli/acp/ test/cli/smokes/ test/cli/serve/ test/cli/run/run-process.test.ts test/cli/mcp-add.test.ts test/cli/help/help-snapshots.test.ts" | ||
| # altimate_change start — wire trace-golden's E2E oracle into the dedicated subprocess pass (its cliIt.live scenario tests are skipped by OPENCODE_SKIP_SUBPROCESS=1 in the main pass above and were previously never run in CI at all) | ||
| SUBPROCESS_PATHS="test/cli/acp/ test/cli/smokes/ test/cli/serve/ test/cli/run/run-process.test.ts test/cli/mcp-add.test.ts test/cli/help/help-snapshots.test.ts test/altimate/trace-golden/" |
There was a problem hiding this comment.
WARNING: test/altimate/trace-golden/ does not exist at this PR's HEAD
This step runs with working-directory: packages/opencode, so the added path resolves to packages/opencode/test/altimate/trace-golden/ — which does not exist (neither does test/altimate/trace-golden/ at the repo root). The trace-golden harness lives in commit 337144a78 ("test: [de-fork] trace-golden e2e harness"), which is not an ancestor of this PR's HEAD (git merge-base --is-ancestor 337144a78 HEAD → false; git ls-tree -r HEAD has no trace-golden entry). Either bun test errors on the missing path (the subprocess pass then emits no N pass summary line, failing the gate at line 175) or it silently skips the path — in both cases the altimate_change start comment's claim of "wiring trace-golden's E2E oracle into the dedicated subprocess pass" is not realized by this PR. Land the harness commit in this PR, or drop the path from SUBPROCESS_PATHS until that lands.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…ensus lookup - `divergence.ts` `parseNumstatZ` now fails closed (throws) on a malformed record head, a truncated rename record, or a non-numeric count field — matching `census.ts`'s sibling parser. Silently skipping / substituting `""` could drop files and understate totals with no error (CodeRabbit). - 3 new fail-closed tests locking that behavior down. - `replay.ts` census attribution builds a `file → bucket` Map once instead of an O(n) `.find()` per conflicted path (CodeRabbit nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dfb358502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # exit code. --timeout 90000 gives headroom for slow fs/spawn tests.) | ||
| SUBPROCESS_PATHS="test/cli/acp/ test/cli/smokes/ test/cli/serve/ test/cli/run/run-process.test.ts test/cli/mcp-add.test.ts test/cli/help/help-snapshots.test.ts" | ||
| # altimate_change start — wire trace-golden's E2E oracle into the dedicated subprocess pass (its cliIt.live scenario tests are skipped by OPENCODE_SKIP_SUBPROCESS=1 in the main pass above and were previously never run in CI at all) | ||
| SUBPROCESS_PATHS="test/cli/acp/ test/cli/smokes/ test/cli/serve/ test/cli/run/run-process.test.ts test/cli/mcp-add.test.ts test/cli/help/help-snapshots.test.ts test/altimate/trace-golden/" |
There was a problem hiding this comment.
In the inspected commit, repo-wide git ls-tree and rg searches find no test/altimate/trace-golden/ directory or generated artifact—only this workflow reference. Bun reports Failed to scan non-existent root directory for tests when given such a root, so the subprocess pass emits no pass summary and the workflow's later missing-summary check exits 1, making this CI job fail instead of running the intended oracle.
Useful? React with 👍 / 👎.
| * discard stdout on a non-zero exit. | ||
| */ | ||
| function runMergeTree(repoRoot: string, baseSha: string, oursSha: string, targetSha: string): { stdout: string; exitCode: number } { | ||
| const result = spawnSync("git", ["merge-tree", "--write-tree", "--merge-base", baseSha, oursSha, targetSha], { |
There was a problem hiding this comment.
When a conflicted Git path legally contains a newline, this line-oriented invocation emits a C-quoted stage path but physically splits the Auto-merging and CONFLICT messages across lines; the parser then extracts only the prefix before the newline and countConflictRegions fails to read the resulting bogus tree path. git merge-tree -h documents -z as separating paths with NUL characters, so request and parse that machine-readable format to support arbitrary repository paths.
Useful? React with 👍 / 👎.
| ): { byPath: Record<string, number>; total: number } { | ||
| const byPath: Record<string, number> = {} | ||
| let total = 0 | ||
| const CONFLICT_START_RE = /^<<<<<<< / |
There was a problem hiding this comment.
When a conflicted file has Git's supported conflict-marker-size attribute set to anything other than the default seven, its merged blob starts with a different number of < characters, but this exact-seven regex counts zero regions. The gitattributes documentation specifies that conflict-marker-size controls marker length; therefore replay reports an understated conflict-surface KPI for such files instead of detecting the actual regions.
Useful? React with 👍 / 👎.
…CI-safety Critical / major: - `refs.ts` `readBlobsBatch`: dedup input OIDs so `cat-file --batch` emits one record per oid — a duplicate previously desynced the buffer offset and silently corrupted the returned blob map (CodeRabbit CRITICAL). - Real-repo tests (`divergence`/`replay`/`taxonomy`) now `describe.skipIf` when the pinned upstream tags aren't resolvable — CI fetches `--no-tags`, so they'd throw during ref resolution on a clean checkout (CodeRabbit CRITICAL/P1). - `replay.ts`: pin `core.quotepath=false` + `merge.renames`/`renameLimit` on merge-tree and route all 3 git calls through `sanitizedGitEnv()` (strips `GIT_DIFF_OPTS`/`GIT_EXTERNAL_DIFF`) — quoted paths were missing the census lookup and inherited env could perturb counts (CodeRabbit Major + WARNINGs). - `divergence.ts`: pin `diff.indentHeuristic=true` (baseline's value) + the `diff.ignore*` family off, so the config-immunity claim holds for hunk/line counts (CodeRabbit WARNING). Robustness / correctness: - `divergence.ts` `parseNumstatZ` fails closed on malformed head / truncated rename / non-numeric counts (+ 3 tests); `replay.ts` census attribution uses an O(1) Map and rejects a census with a mismatched `upstreamBaseSha` / `taxonomyVersion` (not just `oursSha`). - `census.ts`: validate every unclosed-marker-allowlist entry (a bad `expires` was a silent permanent suppression); document the intentional orphan-`end` tolerance; drop dead `parseArgs` `args`/`void args`. - Fix `baselines/README.md` census headline to match the committed `census.json` (228/84 fork-owned, 236/104 outside-boundary); replace a tautological divergence test assertion with real invariants (unique paths + sub-tally reconciliation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 167c463ed9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const head = resolveRefOrThrow(headRef, repoRoot) | ||
| const baseResolved = resolveRefOrThrow(base, repoRoot) | ||
|
|
||
| const result = spawnSync("git", ["diff", "--numstat", "-M", "-z", baseResolved.sha, head.sha], { |
There was a problem hiding this comment.
When a repository or runner configures a low diff.renameLimit, this -M invocation can disable rename detection for a large change while still exiting successfully, so an upstream-file rename is counted as separate deletion/addition records and changes the reported file, bucket, and added-line budgets. The inspected git diff -h documents -l<n> as “limit rename attempts up to paths,” and git help --config lists diff.renameLimit; pass an explicit -l or -c diff.renameLimit=... as divergence.ts does.
Useful? React with 👍 / 👎.
| ): DivergenceEnvelope { | ||
| const upstreamBase = resolveRefOrThrow(upstreamBaseRef, repoRoot) | ||
| const ours = resolveRefOrThrow(oursRef, repoRoot) | ||
| const upstreamPaths = loadPathsAtRef(upstreamBaseRef, repoRoot) |
There was a problem hiding this comment.
When upstreamBaseRef is a movable branch and another process updates it after line 304 resolves it, this read can use the new tree while the diff and recorded upstreamBaseSha/upstreamBaseTree still use the old commit. The resulting envelope then has reproducible diff totals but incorrect, non-reproducible byBucket attribution; load paths from upstreamBase.sha instead.
Useful? React with 👍 / 👎.
| ` git fetch ${remoteHint} ${ref} --no-tags\n` + | ||
| `Then re-run this command.`, |
There was a problem hiding this comment.
For a missing unqualified branch such as main, following this advertised command commonly creates upstream/main and writes FETCH_HEAD, but it does not create a local main ref, so rerunning with the original --upstream-base main still fails. I verified this in a clean repository with the standard remote wildcard refspec; consistent with git fetch -h describing the argument as a <refspec>, the hint should either include an explicit destination ref or tell the operator to rerun using the resulting remote-tracking ref.
Useful? React with 👍 / 👎.
…cibility - CRITICAL/P1 (fails CI on this branch): ci.yml's SUBPROCESS_PATHS referenced `test/altimate/trace-golden/`, but that directory does not exist on this S1 branch (it lives on defork/trace-golden-e2e). `bun test` reports "Failed to scan non-existent root directory", the subprocess pass emits no summary, and the missing-summary guard exits 1. Remove the path here; its CI wiring correctly lands on the trace-golden PR where the directory exists. - divergence.ts: read taxonomy paths from the RESOLVED `upstreamBase.sha`, not the (possibly movable) ref — a branch advanced between resolution and the ls-tree read would attribute buckets against a newer tree than the diff and the recorded SHA/tree used (reproducible totals, wrong byBucket). - census.ts `computeDiffBudget`: pin `diff.renameLimit` so a low repo/runner config can't silently disable `-M` rename detection and miscount an upstream-file rename as a separate delete+add (skewing file/bucket/line budgets). Mirrors divergence.ts's PINNED_DIFF_CONFIG. - refs.ts: the missing-branch fetch hint suggested `git fetch <remote> <branch>` which only updates the remote-tracking ref, not a local one, so re-running with the same unqualified name still failed. Reword to tell the operator to re-run against `<remote>/<branch>` (or fetch into an explicit local ref). Deferred (pathological-input edge cases, documented for follow-up): merge-tree NUL(`-z`) parsing for conflicted paths containing literal newlines, and honoring a non-default `conflict-marker-size` gitattribute in region counting. 158 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/upstream/replay.test.ts`:
- Around line 12-15: Replace the execSync call in hasRefs with spawnSync
imported from child_process, passing git arguments through an array rather than
interpolating a shell command. Remove the shell-level “|| true” fallback and
determine each reference’s validity from the spawned process exit status,
preserving the existing refs.every behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ebd29b74-24f7-4766-9ba6-e5928d2af202
📒 Files selected for processing (9)
.github/workflows/ci.ymlscript/upstream/baselines/README.mdscript/upstream/census.tsscript/upstream/divergence.test.tsscript/upstream/divergence.tsscript/upstream/replay.test.tsscript/upstream/replay.tsscript/upstream/taxonomy.test.tsscript/upstream/utils/refs.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- script/upstream/baselines/README.md
- script/upstream/taxonomy.test.ts
- script/upstream/divergence.test.ts
- script/upstream/utils/refs.ts
- script/upstream/replay.ts
- script/upstream/divergence.ts
| function hasRefs(...refs: string[]): boolean { | ||
| const root = resolveRepoRoot() | ||
| return refs.every((r) => execSync(`git rev-parse --verify --quiet "${r}^{commit}" || true`, { cwd: root, encoding: "utf-8" }).trim().length > 0) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use spawnSync to prevent potential command injection and avoid shell syntax.
execSync with dynamic string interpolation is flagged by static analysis. While the current arguments are hardcoded, you can future-proof this helper and avoid relying on shell evaluation (|| true) by using spawnSync with an argument array and checking the process exit status.
(Remember to import spawnSync from "child_process" if you apply this fix).
🛡️ Proposed fix
function hasRefs(...refs: string[]): boolean {
const root = resolveRepoRoot()
- return refs.every((r) => execSync(`git rev-parse --verify --quiet "${r}^{commit}" || true`, { cwd: root, encoding: "utf-8" }).trim().length > 0)
+ return refs.every((r) => {
+ const { status } = spawnSync("git", ["rev-parse", "--verify", "--quiet", `${r}^{commit}`], { cwd: root })
+ return status === 0
+ })
}🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 14-14: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/upstream/replay.test.ts` around lines 12 - 15, Replace the execSync
call in hasRefs with spawnSync imported from child_process, passing git
arguments through an array rather than interpolating a shell command. Remove the
shell-level “|| true” fallback and determine each reference’s validity from the
spawned process exit status, preserving the existing refs.every behavior.
Source: Linters/SAST tools
| // Pin diff.renameLimit so a low repo/runner config can't silently disable rename | ||
| // detection (`-M` falls back to no detection past the limit), which would miscount an | ||
| // upstream-file rename as a separate delete+add and skew the file/bucket/line budget. | ||
| // Mirrors divergence.ts's PINNED_DIFF_CONFIG. |
There was a problem hiding this comment.
SUGGESTION: The comment claims this "Mirrors divergence.ts's PINNED_DIFF_CONFIG" but the invocation pins only diff.renameLimit — and even that with a different value (999999 here vs RENAME_LIMIT = 20000 in divergence.ts).
divergence.ts#PINNED_DIFF_CONFIG additionally pins the diff.ignore* family (which directly changes the added/deleted counts this function returns), core.quotepath, diff.algorithm, diff.indentHeuristic, etc.; passes --no-ext-diff --no-textconv so a read-only report can't execute configured helpers; and wraps the call in sanitizedGitEnv() to strip inherited GIT_DIFF_OPTS/GIT_EXTERNAL_DIFF. Census's computeDiffBudget does none of those, so the same (base, head) pair can yield different budgets on a differently-configured runner — the exact asymmetry the new comment denies. Either share PINNED_DIFF_CONFIG + sanitizedGitEnv from divergence.ts, or scope the comment to renameLimit only.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23b5a27cbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // detection (`-M` falls back to no detection past the limit), which would miscount an | ||
| // upstream-file rename as a separate delete+add and skew the file/bucket/line budget. | ||
| // Mirrors divergence.ts's PINNED_DIFF_CONFIG. | ||
| ["-c", "diff.renameLimit=999999", "diff", "--numstat", "-M", "-z", baseResolved.sha, head.sha], |
There was a problem hiding this comment.
When a runner or repository sets diff.algorithm (for example, histogram), this invocation inherits it and can report different added-line budgets for the same commits. I checked Git 2.43's git help --config, which lists diff.algorithm, and reproduced a fixture where --numstat reported +3/-4 with Myers but +4/-5 with Histogram; pin the algorithm and other count-affecting diff options as divergence.ts does.
Useful? React with 👍 / 👎.
| `Bucket/category classification rules changed since the baseline was generated — regenerate the baseline before running --check.`, | ||
| ) | ||
| } | ||
| if (current.upstreamBaseRef !== baseline.upstreamBaseRef || current.upstreamBaseSha !== baseline.upstreamBaseSha) { |
There was a problem hiding this comment.
When the baseline records v1.17.9 but the current invocation uses an equivalent ref such as refs/tags/v1.17.9 or the resolved SHA, this condition rejects the check even though both refs resolve to the same immutable commit/tree and therefore produce identical bucket classification. Compare resolved identity rather than the user-supplied ref spelling so valid --upstream-base <ref> aliases do not block the ratchet.
Useful? React with 👍 / 👎.
| let forkAddedOutsideBoundaryFilesChanged = 0 | ||
| let addedLinesInUpstreamShared = 0 | ||
| let addedLinesInUpstreamSharedNonTest = 0 | ||
| const isTestPath = (p: string) => /(^|\/)(test|tests)\//.test(p) || /\.(test|spec)\.(ts|tsx|js|jsx)$/.test(p) |
There was a problem hiding this comment.
When added upstream-shared lines live under a common test directory such as src/__tests__/, this predicate returns false and adds them to addedLinesInUpstreamSharedNonTest; it similarly misses test suffixes such as .test.mts. The sibling divergence metric explicitly recognizes __tests__ and arbitrary alphabetic test extensions, so the two reports disagree and a future S3 non-test threshold can reject test-only changes as production drift.
Useful? React with 👍 / 👎.
| const unclosedAllowlist = | ||
| opts.unclosedAllowlist ?? | ||
| loadUnclosedAllowlist(opts.unclosedAllowlistPath ?? path.join(repoRoot, "script/upstream/unclosed-marker-allowlist.jsonc")) |
There was a problem hiding this comment.
When --ours points to an older commit and the working-tree allowlist has since changed, the census scans source files from the old tree but applies line-number suppressions from the current checkout. This can either fail a reproducible historical census after harmless line movement or suppress an unrelated unclosed marker that now occupies the same coordinate, while the envelope records only the analyzed commit/tree; load the default allowlist from the resolved ours revision or pin its identity in the provenance.
Useful? React with 👍 / 👎.
| for (const relPath of sortedPaths) { | ||
| const content = filesAtRef.get(relPath)! | ||
| const { blocks, unclosed } = parseMarkerBlocks(relPath, content, upstreamPaths) | ||
| allBlocks.push(...blocks) |
There was a problem hiding this comment.
When an unclosed block matches the allowlist, it is still retained in allBlocks and therefore contributes to total, bucket, category, and file counts even though the committed allowlist explicitly identifies every entry as documentation or test-string false positives rather than real markers. The committed baseline consequently reports 34 such entries in totals.unclosed and includes them in its 1,610-block headline, corrupting the de-fork inventory; exclude applied false positives from the marker population or report them only in the separate application section.
Useful? React with 👍 / 👎.
Review comments addressed (2 rounds)Thanks to @coderabbitai, @chatgpt-codex-connector, and @kilo-code-bot. Resolutions: Round 2 —
Round 3 —
Deferred (documented in the round-3 commit body): merge-tree 158 pass, typecheck clean. |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
PINEAPPLE
Summary
Stage S1 of the de-fork spike (spec:
docs/internal/2026-07-18-defork-spike-spec.md, Codex-consensus-approved). Builds the measurement foundation for shrinking the fork's upstream-shared footprint so future upstream merges get cheap. Zero product-behavior change — all new code is fork-owned tooling inscript/upstream/.Four tools sharing one taxonomy module:
taxonomy.ts— 3-bucket classification (upstream_shared/fork_owned/fork_added_outside_boundary) against the pinnedv1.17.9upstream tree, with upstream-existence precedence (10.opencode/overlap paths pinned by a generated oracle test).census.ts— stack-basedaltimate_changemarker-block parser reading from the git tree (not the working FS), versioned provenance-pinned JSON envelope, and a multiset{file,contentHash}ratchet (--check) with quantity-scoped committed exemptions.divergence.ts— files/hunks/±lines vs the upstream base viagit diff --numstat(git-exact; matchesgit --shortstat). Diff config is pinned (diff.algorithm=myers,interHunkContext=0,core.quotepath=false,--no-ext-diff --no-textconv,GIT_DIFF_OPTSstripped) so numbers reproduce across git environments.replay.ts— explicit-base three-way merge viagit merge-tree --write-tree --merge-base(object-db only, never mutates the working tree), with fail-closed region/binary reads; requires git ≥ 2.40.Why: the last upstream bump (v1.4.0→v1.17.9) took a multi-week, 7-checkpoint campaign. Before shrinking the entangled surface we need a reproducible, gaming-resistant measure of it. The headline KPI is conflict surface on a replayed upstream merge, not marker count.
Baseline numbers (real runs, HEAD
8a50ec7f55vsv1.17.9)upstream_fixblocks (upstreaming candidates).diff.algorithm=patience.<<<<<<<regions, and 4 are binary (2 PNG, 1 TTF, 1 WOFF2). Auto-merge: 212 attempted, 94 truly clean.Test Plan
bun testinscript/upstream: 155 pass / 0 fail (nesting, unmatched-marker handling, taxonomy overlap oracle, multiset ratchet pass+fail, envelope determinism, numstat++/--content regression, merge-tree parsing against a real fixture repo, honest replay metrics + rename-no-double-count).git --shortstatground truth; config-immunity verified (oracle holds under an injecteddiff.algorithm=patience).bunx tsc --noEmit -p script/upstream: clean. Allscript/upstream/*.tsare text (no NUL bytes).Note: the CI census-ratchet wiring in
.github/workflows/ci.ymllands in a small follow-up (that file is being co-edited by a sibling de-fork stage); this PR is the tooling + baselines.Checklist
Closes #1010
Summary by cubic
Adds S1 de-fork baseline metrics and merge-replay tooling to measure and shrink our upstream-shared footprint. No product behavior changes; tooling lives in
script/upstream/. CI now fetches the upstream base tag; the trace-golden subprocess wiring was removed on this branch.New Features
taxonomy.ts: 3-bucket classifier (upstream_shared/fork_owned/fork_added_outside_boundary) againstv1.17.9, with upstream-existence precedence and pinned overlap tests.census.ts: parsesaltimate_changeblocks from the git tree, emits a versioned JSON envelope, and enforces a multiset{file,contentHash}ratchet with quantity-scoped exemptions; unclosed markers fail unless allowlisted.divergence.ts: counts files/hunks/±lines viagit diff --numstat -M -zwith pinned diff config for reproducible numbers.replay.ts: simulates an explicit-base three-way merge viagit merge-tree --write-tree --merge-base(no working-tree changes), attributes conflicts to census buckets; requires git ≥ 2.40.script/upstream/baselines/2026-07-18/(census, divergencev1.17.9, replayv1.18.3). Snapshot: Census 1,146 shared blocks; Divergence 5,283 files, +499,181/−740,989; Replay 651 conflicted paths (118 textual, 4 binary), 94 clean auto-merges.Bug Fixes
v1.17.9tag; removed non-existenttest/altimate/trace-golden/subprocess path that broke this branch’s CI.divergence.ts: reads taxonomy paths from the resolvedupstreamBase.sha(not the movable ref) for consistent attribution.census.ts:computeDiffBudgetpinsdiff.renameLimitso rename detection stays on and counts remain stable.refs.ts: improved missing-branch guidance to re-run against<remote>/<branch>(or fetch into a local ref).refs.ts: dedups OIDs inreadBlobsBatchto avoid buffer desync and blob map corruption.--no-tags).replay.ts: pinnedcore.quotepath=false,merge.renames, andrenameLimit; all git calls use a sanitized env (GIT_DIFF_OPTS/GIT_EXTERNAL_DIFFstripped). Rejects census with mismatchedupstreamBaseSha/taxonomyVersion.divergence.ts: pinneddiff.indentHeuristic=trueand disableddiff.ignore*;parseNumstatZfails closed on malformed/truncated records and non-numeric counts.census.ts: validates each unclosed-marker allowlist entry; documents orphan-endtolerance.census.jsonheadline counts.Written for commit 23b5a27. Summary will update on new commits.
Summary by CodeRabbit