fix(release-readiness): close three coherence defects found on dev - #1810
fix(release-readiness): close three coherence defects found on dev#1810lidge-jun wants to merge 2 commits into
Conversation
…ctor Coherence review of the merged stack found the mutation paths weaker than the compatibility report they claim to honor. inspect.ts requires exact table SQL, exact column metadata and every canonical index; protection.ts and maintenance.ts each rechecked column NAMES only. The lock serializes OpenCodex against itself but not against Codex or another SQLite writer, so a migration landing between the outer inspection and the locked write was a real window. Reproduced on dev HEAD by dropping idx_logs_ts in that gap: Protect installed a row-dropping trigger and returned ok:true, and Reclaim vacuumed pages, both on a database the inspector reported as unsupported/monitor-only. hasCurrentLogsSchema is now exported from inspect.ts and both mutation paths delegate to it, so the pre-write gate and the compatibility report can no longer disagree. Regression drives the exact window.
…uplicates
Two findings from the dev coherence review.
1. Preview releases were unreleasable. selectReleaseBaseline picked the newest
tag of either channel without regard to reachability, so a preview selected
the newest STABLE tag - which lives on main and is not reachable from
preview - and the ancestry guard then threw. Verified against the real
repository:
preview: without-filter=v2.21.0 (ancestor=false) -> with-filter=v2.19.0
preview-from-dev: without-filter=v2.21.0 (ancestor=false) -> with-filter=v2.17.0
Selection now filters to tags actually reachable from the target. The
ancestry guard remains as a fail-closed assertion for any future caller that
skips the filter.
2. The duplicate-closure gate stripped only closed HTML comments, while the
quality parser treats an unclosed comment as hiding everything through EOF.
Two issues whose VISIBLE bodies differ could therefore match on text GitHub
never renders, and the workflow closes on that match. Now uses the same
through-EOF semantics; verified that hidden text no longer matches while
ordinary visible signatures still do.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe changes update unterminated HTML-comment normalization, ancestry-aware release baseline selection, and shared Codex Log Guard schema validation. Tests cover unreachable release tags and protection behavior when the canonical log index is missing. ChangesSignature normalization
Release baseline selection
Codex Log Guard schema validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ 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 |
![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: ff1b2e5418
ℹ️ 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".
| // quality parser treats it. Matching only `<!-- ... -->` let hidden text | ||
| // become the "exact shared signature" that closes an issue: two reports | ||
| // whose visible bodies differ could match on text GitHub never renders. | ||
| .replace(/<!--[\s\S]*?(?:-->|$)/g, " ") |
There was a problem hiding this comment.
When an unclosed comment spans multiple lines, this replacement sees only one line at a time because extractStrongFailureSignatures splits the text afterward and calls normalizeSignatureLine separately for each line. For example, a body containing <!-- followed on the next line by a strong failure signature still returns that hidden signature, allowing two visibly different issues to be auto-closed as duplicates. Strip comments from the complete body through EOF before splitting it into candidate lines.
Useful? React with 👍 / 👎.
| const probe = await runCommand(["git", "merge-base", "--is-ancestor", tag, target]); | ||
| ancestryCache.set(tag, probe.exitCode === 0); | ||
| } | ||
| const baseline = selectReleaseBaseline(version, tags, tag => ancestryCache.get(tag) === true); |
There was a problem hiding this comment.
When a preview branch cannot reach the newest stable tag, this correctly selects an older reachable baseline for GitHub-generated notes and releaseCommits, but buildReleaseNotes later calls selectReleaseBaseline(input.version, input.tags) without the reachability predicate. The published Full Changelog URL and reported baseline therefore still name the newer unreachable stable tag even though the entries were built from another range. Pass this selected baseline into buildReleaseNotes, or pass only the reachable tag set, so every stage uses the same range.
Useful? React with 👍 / 👎.
1 participant
Summary
Coherence review of everything that landed on
devthis session found three defects that the per-PR audits could not see, because each only appears once the changes are combined or exercised against the real repository. All three are fixed here with activating regressions.1. Log Guard mutations were weaker than the compatibility report they honor (High)
inspect.tsclassifies a database as compatible only with exact table SQL, exact column metadata, and every canonical index. Both mutation paths rechecked column names only inside their write transaction.The Log Guard lock serializes opencodex against itself; it does not stop Codex or another SQLite writer. So a migration landing between the outer inspection and the locked write is a real window. Reproduced on dev HEAD by dropping
idx_logs_tsin that gap:ok: trueboth on a database the inspector reports as
unsupported/ monitor-only.hasCurrentLogsSchemais now exported frominspect.tsand both paths delegate to it, so the pre-write gate and the compatibility report cannot disagree.2. Preview releases were unreleasable (High)
selectReleaseBaselinepicked the newest tag of either channel without regard to reachability. A preview therefore selected the newest stable tag — which lives onmainand is not reachable frompreview— and the ancestry guard added in #1740 then threw. Verified against this repository's real tags and refs:The moment the stable lineage moved ahead, no preview could be released at all. Selection now filters to tags actually reachable from the target; the ancestry guard stays as a fail-closed assertion for any future caller that skips the filter.
3. Hidden text could forge a duplicate closure (Medium)
issue-triage-autoclose.cjsstripped only closed<!-- ... -->comments, while the quality parser treats an unclosed comment as hiding everything through EOF. Two issues whose visible bodies differ could therefore match on text GitHub never renders, andissue-triage.ymlcloses on that match. Now uses the same through-EOF semantics.Verification
Ground truth first, at dev HEAD
8a0de6c44, full suite on an isolated Linux checkout:All 7 failures are
Cannot find package 'react'/react/jsx-dev-runtimefromgui/sources, because the scratch clone installs no GUI dependencies. None touch routing, catalog, or log-guard.After these fixes, on the same host at this branch head:
Each new regression was reproved to fail without its source change.
Three independent
gpt-5.6-solexplorers audited dev in parallel across catalog/discovery, the log-guard stack, and CI-script governance. Their "checked and sound" findings are worth recording too: inspection cache invalidation works correctly (every mutation changes the DB/WAL/SHM stamp), the lock correctly serializes Protect against Reclaim, and the changelog builder renders the real 12-commit and 23-commit ranges with zero coverage errors.Two lower-severity findings from those lanes are not fixed here and are recorded rather than silently dropped: a late slug collision can misattribute capabilities to an unroutable custom model, and prefix stripping runs before sibling enrichment so a Cloudflare-style
@cf/...id loses its sibling capabilities. Both are pre-existing interactions rather than regressions from this session, and each deserves its own audited change.Checklist
Summary by CodeRabbit
Bug Fixes
Tests