diff --git a/.claude/skills/issues/SKILL.md b/.claude/skills/issues/SKILL.md index c04936235..cbd39b8a5 100644 --- a/.claude/skills/issues/SKILL.md +++ b/.claude/skills/issues/SKILL.md @@ -64,9 +64,12 @@ paragraph; put the smallest next action in **Detail / next action**. open or resolved table as appropriate. - IDs are monotonic and never reused — always allocate from the `issues:next-id` marker and bump it. - Escape `|` inside cell text (write `\|`) so the markdown table stays intact. -- This file uses `merge=union` in `.gitattributes`. Never resolve a conflict by taking one side - wholesale — that drops the other agent's rows. `npm run check:outstanding-issues` fails on - duplicate IDs or a stale next-id marker. +- This file deliberately has **no** merge driver, so an overlapping edit conflicts loudly. + `merge=union` was tried and removed: it concatenated both sides silently, duplicating rows and + the `next-id` marker (`#133`). Never resolve a conflict by taking one side wholesale — that + drops the other agent's rows. Rebuild from `origin/main` and re-apply only the rows you + changed. `npm run check:outstanding-issues` fails on duplicate IDs, a stale next-id marker, + or a merge driver reappearing. - Respect the repo's RAG/clinical/privacy flagging rules if an item _itself_ touches a protected surface — recording it here is fine, but acting on it later still needs the usual gate. diff --git a/.gitattributes b/.gitattributes index 94ba09624..a1a97178f 100644 --- a/.gitattributes +++ b/.gitattributes @@ -13,7 +13,13 @@ # but also reintroduced babysit twins that failed the ledger guard). docs/branch-review-ledger.md merge=ledger -# Outstanding-issue rows are also append-mostly across concurrent agent sessions. -# Union merge preserves both sides' rows; scripts/check-outstanding-issues.mjs -# still fails on duplicate IDs because row-level union alone cannot allocate IDs. -docs/outstanding-issues.md merge=union +# Outstanding-issue rows deliberately get NO merge driver. Union was tried (PR +# #1416) and removed: ledger #133 recorded that it "concatenates conflicting +# hunks, so two sides each bumping the marker produce two `next-id` lines, +# corrupting the file silently where a conflict would fail loudly", and on +# 2026-07-30 (PR #1430) it duplicated the entire open-items table on four +# separate merges. Unlike docs/branch-review-ledger.md this file allocates IDs +# by read-modify-write, so concurrent appends need manual renumbering whatever +# the driver does — union bought nothing and hid the overlap. Default 3-way +# merge conflicts honestly instead; check-outstanding-issues.mjs enforces that +# no driver is set, and still gates IDs, the marker and row shape. diff --git a/AGENTS.md b/AGENTS.md index 583e1fc44..e44efbdff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -618,8 +618,11 @@ named PR). Future process only. - Start from a fresh `origin/main` worktree/branch (`newtask`); do not pile new work onto a stale head that already shares hot files with the open queue. - Treat `docs/branch-review-ledger.md` and `docs/outstanding-issues.md` as hot shared files. - Both use `merge=union`. Append with `npm run ledger:append` / the `/issues` skill — never - hand-write ledger rows, and never resolve an outstanding-issues conflict by taking one side + `docs/branch-review-ledger.md` uses the custom `merge=ledger` driver (union with exact-row + dedupe); `docs/outstanding-issues.md` deliberately has **no** driver, so overlapping edits + conflict loudly rather than being silently concatenated — union was tried and removed + (`#133`). Append with `npm run ledger:append` / the `/issues` skill — never hand-write + ledger rows, and never resolve an outstanding-issues conflict by taking one side wholesale. `npm run check:outstanding-issues` fails on duplicate IDs or a stale `issues:next-id` marker. - Before calling GitHub `DIRTY`/`CONFLICTING` a real conflict, run diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index ef5f14f16..77275d7ec 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -157,7 +157,7 @@ removed after current-main verification; it is not missing recommended work. | #130 | P2 | issue | PR #1396 merged shared phone-chrome behaviour without its own declared physical-device gate | **Outcome:** a shared-chrome PR does not merge with a self-declared merge prerequisite left undone, or the ledger records that it did. **Detail:** PR #1396 ("overlay the phone header so hiding it never moves content") repeatedly stated in its own body and PR comments that `docs/phone-chrome-physical-acceptance.md` "genuinely applies before merge" because local Chromium cannot certify Safari chrome-minimisation or cold-launch PWA paint (invariant 23) — restated at least three times across the review thread, including after the final `a638b66e`/`f7347144` fix. It merged at 06:49:55 anyway. Checked 2026-07-30: `docs/phone-chrome-physical-acceptance.md` on `main` is still the blank checklist template — every "Result / evidence" cell is empty, no PR comment attaches a filled-in copy or device evidence, and no existing ledger row (`#120`, `#122`) covers this gap. Related but distinct: one Codex thread on this PR also names a still-missing guard — a pre-paint/cold-load hydration test comparing content position before and after hydration, which the author explicitly said they would "rather file it than ship a test that looks like it covers the window and does not" — and that filing never happened either. **Next:** run the physical-device matrix in `docs/phone-chrome-physical-acceptance.md` against `main`'s current tip on a real iPhone (Safari tab + cold-launch PWA, light/dark, portrait/landscape) and commit the filled-in evidence; separately, add the pre-paint/cold-load Playwright pattern this PR's own review identified as missing. **Stop:** do not treat this PR's extensive Codex/CI remediation (13 findings fixed, 9 threads resolved) as a substitute for the physical-device proof — headless Chromium was explicitly stated as unable to certify the two things this checklist exists for. | PR #1396 (merged 2026-07-30); session 2026-07-30 PR babysit | 2026-07-30 | | #131 | P2 | issue | `@codex fix` produces commits that never reach the repository | **Outcome:** a finding routed to Codex is either fixed on the branch or visibly not fixed. **Detail:** on 2026-07-30 PR #1400, eleven `@codex fix` dispatches produced at least eleven commits — `f632ba2`, `bb27822`, `1ab2c40`, `749a183`, `08a6b7a`, `ed6be22`, `6e07327`, `9353757`, `c2e56d0` and others — every one reported as committed on a branch named `work` with "stacked pull request metadata" created. **None is fetchable:** `git cat-file -e` fails locally and `git fetch origin ` fails for each, and no PR carries them. The same single finding was rewritten four separate times under different SHAs, none landing. Its reports also show it could not execute tests (`node_modules` absent, Node 20 vs the required 24), so the claims were unverified as well as unlanded. The danger is that it reads as success: threads get authoritative-looking "Summary / Testing" replies while the branch is unchanged. **Next:** decide whether the Codex connector is expected to push to the PR branch and, if so, why it is writing to a detached `work` branch instead; until then treat `@codex fix` as advisory only and land fixes another way. **Stop:** never resolve a review thread on the strength of a Codex report — verify against the actual ref content first, per AGENTS.md. | PR #1400; session 2026-07-30 | 2026-07-30 | | #132 | P3 | issue | Both client-side push guards are inert for agent pushes | **Outcome:** the format and auto-merge guards protect every push, or their blind spot is explicit. **Detail:** `scripts/guard-push.mjs` printed `auto-merge: gh not available — auto-merge check skipped (fail-open)` for pushes from a remote agent environment, so the auto-merge race sentinel never evaluated; and `core.hooksPath` is set only by a local `npm install`, so an agent pushing from its own checkout bypasses `.githooks/pre-push` entirely. Both guards therefore protect exactly the environment least likely to break the rule, which is why the AGENTS.md format-before-push instruction is still load-bearing even though the tooling now exists. Observed directly on PR #1400: a push landed while auto-merge was armed with nothing to stop it. **Next:** provide `gh` (or a token-based equivalent) in agent environments so the sentinel can evaluate. **Do not move the format check into `pull_request_target`** — that context carries secrets and a write token, and a format check must execute PR-head code including this repo's now-loadable dynamic `prettier.config.*`, which is the classic privileged-context vector; `.github/workflows/pr-policy.yml` deliberately checks out only `github.workflow_sha` for exactly this reason. Formatting is already enforced server-side by `Static PR checks` running `format:check` on ordinary `pull_request` CI, so the guard's only unique value is failing fast before the push — nothing to duplicate. The auto-merge sentinel reads PR metadata only and could safely live in `pull_request_target` if it is ever worth moving. | PR #1400; session 2026-07-30 | 2026-07-30 | -| #133 | P3 | rec | `docs/outstanding-issues.md` conflicts on nearly every `main` advance | **Outcome:** two agents editing different rows of this ledger do not conflict. **Detail:** the table is padded to fixed column widths, so a single row's edit re-pads all 59 open rows and git sees the whole table as one changed hunk. On 2026-07-30 this file conflicted twice within an hour on PR #1400, and each conflict silently stopped **all** CI on that PR (`#116`) — so the cost is not the merge itself but the invisible loss of every check while it lasts. Both conflicts were mechanical: only two rows differed semantically out of 58. **Next:** stop padding this table (Prettier will still render it readably, and one-row edits become one-line diffs), or split the open items into per-row files. **Do not** apply a `merge=union` driver — tested 2026-07-30 and it is worse: union concatenates conflicting hunks, so two sides each bumping the marker produce two `next-id` lines, corrupting the file silently where a conflict would fail loudly. `#112`'s new `check:outstanding-issues` gate catches the corrupted result but does not prevent the conflict. | PR #1400; session 2026-07-30 | 2026-07-30 | +| #133 | P3 | rec | Ledger conflicts on nearly every `main` advance (union driver removed) | **Outcome:** two agents editing different rows of this ledger do not silently corrupt it. **Driver half RESOLVED 2026-07-30:** `merge=union` is removed from `.gitattributes` and `check:outstanding-issues` now requires that **no** driver is set, so a reappearance is a red gate (proved by reintroducing it: `must have NO merge driver (found merge=union)`). This row already recorded union as worse — "two sides each bumping the marker produce two `next-id` lines, corrupting the file silently where a conflict would fail loudly" — but the attribute stayed in place and the gate _mandated_ it, so the repo's own tested conclusion was contradicted by its own config. PR #1430 confirmed the cost at scale: four merges in one session, each reporting success while duplicating the **entire** open-items table (`#059 appears 2 times (lines 101, 166)` and so on), every one needing a manual rebuild from `origin/main`. Union also makes `git merge-tree` report a clean tree, so the pre-merge conflict check cannot warn. Unlike `docs/branch-review-ledger.md`, this file allocates IDs by read-modify-write, so concurrent appends need manual renumbering whatever the driver does — union bought nothing. `AGENTS.md`, `docs/process-hardening.md`, `.claude/skills/issues/SKILL.md` and `docs/scripts-index.md` are updated to match. **Still open — the conflict frequency itself:** the table is padded to fixed column widths, so one row's edit re-pads every open row and git sees the whole table as one changed hunk; on 2026-07-30 it conflicted twice within an hour on PR #1400, and each conflict silently stopped **all** CI on that PR (`#116`). **Next:** stop padding this table (Prettier still renders it readably, and one-row edits become one-line diffs), or split the open items into per-row files. **Stop:** do not reintroduce a merge driver here; if concurrent-append pain returns, write a dedupe driver like `merge=ledger`'s rather than stock union. | `.gitattributes`; `scripts/check-outstanding-issues.mjs`; PR #1400; PR #1430 | 2026-07-30 | | #134 | P2 | issue | Ledger union-merge driver is absent wherever `npm install` was skipped | **Outcome:** the ledger's union-merge protection is present wherever a merge happens, or its absence is loud. **Detail:** `.gitattributes` declares `docs/branch-review-ledger.md merge=ledger`, but the driver itself lives in git _config_, installed by `postinstall` -> `scripts/install-git-hooks.mjs`. A container that skips `npm install` (this repo's remote agent sessions do — the session hook reports "node_modules matches the lockfile, skipping install") therefore has the attribute without the driver, and git silently falls back to an ordinary merge. On 2026-07-30 a `git merge origin/main` on PR #1424 produced **conflict markers inside the append-only ledger** at three lines; `git merge` itself did not name the file, so only `npm run check:branch-review-ledger` caught it. Committing that would have corrupted the file the guard exists to protect. **Next:** make the absence loud — have `check:branch-review-ledger` (already in `verify:cheap` and `static-pr`) fail when `.gitattributes` declares `merge=ledger` but `git config merge.ledger.driver` is unset, so the environment is caught before a merge rather than after. `npm run hooks:install` is the one-line fix once detected. **Stop:** never trust a `merge=union`-style attribute to be active just because `.gitattributes` declares it; the driver is per-checkout config. | PR #1424; session 2026-07-30 | 2026-07-30 | | #135 | P2 | rec | UI scope overfires: `ui_changed` matches every `src/app` path | **Outcome:** a change that cannot alter a rendered journey stops paying the longest job in CI. **Detail:** `uiPatterns` in `scripts/ci-change-scope.mjs` matches all of `src/app`, so an edit confined to `src/app/api/**` sets `ui_changed` and runs the full Chromium gate — measured 2026-07-30 at 15m26 of an 18m36 run, the entire critical path. **Next:** decide whether API-only diffs can be excluded. **Not done blind, deliberately:** the journeys exercise a production build that serves those routes, so a naive exclusion can hide a real regression; this needs a decision plus a compensating check, not a quieter filter. Sharding `ui-critical` (PR #1427) cut the cost of over-firing but did not remove it. | `scripts/ci-change-scope.mjs`; CI runs 30520443076 / 30519912667; session 2026-07-30 | 2026-07-30 | | #136 | P2 | rec | Playwright's isolated production build cannot reuse Next's build cache | **Outcome:** the fixed ~2 min build stops being repaid on every UI job and every shard. **Detail:** `scripts/run-playwright.mjs` builds into `.next-playwright/${pid}-${Date.now()}/dist`, so Next's webpack filesystem cache (which lives under `distDir`) is cold every run — ~1m56 of the 15m26 `Chromium production journeys` step measured 2026-07-30, and a larger share of the sharded critical path. The `build` job already caches `.next/cache` correctly; this path simply cannot hit it. **Next:** allow the run root to be pinned via an env var (defaulting to today's behaviour) and cache it in `ui-critical`. **Blocker:** the runner removes the run root on every exit path, which `docs/testing.md` states as a contract, so caching needs an explicit keep flag and must not ship without executing the runner. | `scripts/run-playwright.mjs`; `docs/testing.md`; session 2026-07-30 | 2026-07-30 | diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 5cf6e45c8..c3f19a64b 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -86,10 +86,12 @@ artifact before release; see Future-process only — do not mutate unrelated active PRs unless explicitly asked. - **Outstanding-issues concurrency (`#112`):** the structural gate landed in PR #1410 (`npm run check:outstanding-issues` in `verify:cheap` / CI `static-pr` — duplicate IDs, - both-tables, stale `issues:next-id`, malformed rows). PR #1416 adds `merge=union` in - `.gitattributes` and a runtime attribute check so concurrent appends keep both sides' - rows; union merge still cannot allocate unique IDs, so the structural gate remains - required. + both-tables, stale `issues:next-id`, malformed rows). PR #1416 added `merge=union` in + `.gitattributes`; it is now **removed** and the runtime attribute check inverted to require + no driver at all. Union could not allocate unique IDs either, and it concatenated + conflicting hunks instead of failing — two marker bumps became two `next-id` lines (`#133`), + and on 2026-07-30 it duplicated the whole open-items table on four merges (PR #1430). + Default 3-way merge conflicts loudly instead; the structural gate remains required. - **Silent CI on conflicted PRs (`#116`):** when GitHub cannot build `refs/pull//merge`, every `pull_request` workflow is skipped with no failing check. `.github/workflows/pr-mergeability.yml` checks trusted `pull_request_target` events and diff --git a/docs/scripts-index.md b/docs/scripts-index.md index 425b77498..aaca65923 100644 --- a/docs/scripts-index.md +++ b/docs/scripts-index.md @@ -18,7 +18,7 @@ migration has shipped (see `docs/maturity-backlog-workorders.md` L1). | `dev-free-port.mjs`, `ensure-local-server.mjs` | Project-stable localhost port selection + background server ensure | | `check-node-engine.cjs`, `install-git-hooks.mjs`, `guard-push.mjs`, `guard-next-build.mjs` | Install/preflight guards | | `ci-change-scope.mjs`, `ci-triage.mjs`, `pr-policy.mjs`, `pr-mergeability.mjs` | CI change classification + PR policy + conflict signal (self-tested via `check:ci-scope`/`check:ci-triage`/`check:pr-policy`/`check:pr-mergeability`) | -| `check-outstanding-issues.mjs`, `check-pr-mergeability-workflow.mjs` | Outstanding-issues ID/marker/union guard + PR mergeability workflow contract | +| `check-outstanding-issues.mjs`, `check-pr-mergeability-workflow.mjs` | Outstanding-issues ID/marker/no-driver guard + PR mergeability workflow contract | | `check-installed-lock-parity.mjs`, `phone-chrome-plan.mjs`, `verify-phone-chrome.mjs` | Lock-trust preflight plus change-scoped phone contracts, ownership journeys, and smart full-UI escalation | | `final-merge-audit.mjs` | Fail-closed local merge-tree audit; explicit provider mode adds PR/check/thread/tree/deployment proof | | `child-process-result.mjs`, `cli-utils.ts`, `productivity-core.mjs` | Shared helpers | diff --git a/scripts/check-outstanding-issues.mjs b/scripts/check-outstanding-issues.mjs index 3d681fee6..72e0f20b0 100644 --- a/scripts/check-outstanding-issues.mjs +++ b/scripts/check-outstanding-issues.mjs @@ -2,11 +2,14 @@ // Structural gate for docs/outstanding-issues.md. // // Ledger #112. The `issues:next-id` marker is a plain HTML comment that every -// editor read-modify-writes with no lock. The file now has `merge=union` (PR -// #1416), which preserves concurrent row appends the same way as -// docs/branch-review-ledger.md, but union merge cannot allocate unique IDs — -// two agents can still collide, and a hurried conflict resolution can still -// take one side wholesale. On 2026-07-29 that happened three times in one hour +// editor read-modify-writes with no lock. A `merge=union` driver was tried (PR +// #1416) and removed: unlike docs/branch-review-ledger.md this file allocates +// IDs by read-modify-write, so union could not allocate unique IDs either, and +// it silently concatenated conflicting hunks — two marker bumps became two +// `next-id` lines (#133), and on 2026-07-30 the whole open-items table was +// duplicated on four merges (#1430). This gate now requires that NO driver is +// set, so overlapping edits conflict loudly. A hurried conflict resolution can +// still take one side wholesale. On 2026-07-29 that happened three times in one hour // on a single PR, and nothing noticed: no gate read this file's structure at // all. This structural gate is what makes those failures loud. // @@ -404,6 +407,36 @@ function selfTest() { console.log("outstanding-issues self-test passed."); } +/** + * The only acceptable state for this file's `merge` attribute. + * + * Git distinguishes three non-driver states, and they are NOT interchangeable + * (see gitattributes, "merge"): *Unspecified* — no pattern matches — is the + * documented default 3-way text merge, which is the contract here. *Unset* + * (`-merge`) instead takes the current branch's version and declares the merge + * conflicted, so every two-sided edit becomes a manual resolution — a different + * regression from a driver, but a regression all the same, and one a global or + * future attributes file could introduce while this gate stayed green. A named + * driver (`union`, `ledger`, …) is the case #133 removed. + * + * Exported so the distinction is unit-tested rather than only reasoned about. + */ +export function mergeAttributeProblem(mergeAttribute) { + if (mergeAttribute === "unspecified") return null; + if (mergeAttribute === "unset") { + return ( + `${ISSUES_PATH} must leave \`merge\` unspecified (found \`-merge\`, i.e. Unset) — ` + + "Unset takes the current branch's version and declares a conflict instead of running the " + + "default 3-way merge, so drop the negated attribute rather than adding one (ledger #133)" + ); + } + return ( + `${ISSUES_PATH} must have NO merge driver (found merge=${mergeAttribute || "empty"}) — ` + + "remove it from .gitattributes so overlapping edits conflict loudly instead of " + + "silently concatenating both sides (ledger #133)" + ); +} + function effectiveMergeAttribute() { const output = execFileSync("git", ["check-attr", "merge", "--", ISSUES_PATH], { encoding: "utf8", @@ -419,13 +452,12 @@ function main() { } const markdown = readFileSync(ISSUES_PATH, "utf8"); const problems = checkIssues(markdown); - const mergeAttribute = effectiveMergeAttribute(); - if (mergeAttribute !== "union") { - problems.push( - `${ISSUES_PATH} must resolve to merge=union (found ${JSON.stringify(mergeAttribute || "unset")}) — ` + - "set it in .gitattributes so concurrent appends keep both sides' rows", - ); - } + // A merge driver on this file is a regression, not an improvement: union + // concatenated conflicting hunks and duplicated the whole table rather than + // failing (#133, and four times on PR #1430). Honest conflicts are the + // contract; ids still need manual renumbering either way. + const mergeProblem = mergeAttributeProblem(effectiveMergeAttribute()); + if (mergeProblem) problems.push(mergeProblem); if (problems.length > 0) { console.error(`${ISSUES_PATH} check FAILED:`); for (const problem of problems) console.error(` - ${problem}`); @@ -439,7 +471,7 @@ function main() { const open = rows.filter((row) => row.table === "open").length; console.log( `Outstanding-issues guard passed: ${rows.length} rows (${open} open, ${rows.length - open} archived), ` + - `unique ids, next-id=${nextId} above the highest, union merge active.`, + `unique ids, next-id=${nextId} above the highest, no merge driver.`, ); } diff --git a/tests/repo-hygiene.test.ts b/tests/repo-hygiene.test.ts index 7b3abbcd9..5ebe61c0c 100644 --- a/tests/repo-hygiene.test.ts +++ b/tests/repo-hygiene.test.ts @@ -32,6 +32,7 @@ import { sanitizeCell, } from "../scripts/branch-review-ledger.mjs"; import { validateLedger } from "../scripts/check-branch-review-ledger.mjs"; +import { mergeAttributeProblem } from "../scripts/check-outstanding-issues.mjs"; describe("check-env-parity name parsing", () => { it("extracts UPPER_SNAKE schema keys from env.ts-style text", () => { @@ -548,3 +549,30 @@ describe("branch-review-ledger guard", () => { expect(validateLedger({ ...valid, ledger: legacy }).failures).toEqual([]); }); }); + +describe("outstanding-issues merge attribute", () => { + // Ledger #133 removed `merge=union` so overlapping edits conflict loudly rather + // than being silently concatenated. Git's three non-driver states are not + // interchangeable, and only one of them is that contract. + it("accepts an unspecified attribute, the documented default 3-way merge", () => { + expect(mergeAttributeProblem("unspecified")).toBeNull(); + }); + + it("rejects `-merge`, which conflicts every two-sided edit instead of merging", () => { + // Unset is not Unspecified: it takes the current branch's version and declares + // a conflict. `git check-attr` reports it as "unset", and an earlier revision of + // this guard accepted that string while printing "no merge driver". + expect(mergeAttributeProblem("unset")).toMatch(/must leave `merge` unspecified/); + }); + + it("rejects a named driver, including the one #133 removed", () => { + expect(mergeAttributeProblem("union")).toMatch(/must have NO merge driver/); + expect(mergeAttributeProblem("ledger")).toMatch(/must have NO merge driver/); + }); + + it("rejects an empty reading rather than treating it as absence", () => { + // An empty string means check-attr output did not parse — silently accepting it + // would make the whole check vacuous. + expect(mergeAttributeProblem("")).toMatch(/must have NO merge driver/); + }); +});