Skip to content

issues: capture the PR #1316 follow-ups that outlived the merge - #1365

Merged
BigSimmo merged 6 commits into
mainfrom
claude/top-search-design-mockups-w53znc
Jul 28, 2026
Merged

issues: capture the PR #1316 follow-ups that outlived the merge#1365
BigSimmo merged 6 commits into
mainfrom
claude/top-search-design-mockups-w53znc

Conversation

@BigSimmo

Copy link
Copy Markdown
Owner

Summary

#096 — seven Codex follow-up PRs carry unrun fixes.ff5b682, 77cfe12, 9840ed9, 81ffb86, a5d6561, 967e16c, e544d0d were authored into separate follow-up PRs rather than the feature branch, so none is in the merged 4bcfeb90. Their content is real — therapy retry-waiter scoping, adoption-gate root-path modes, favourites hub counts, document status derivation — but every one reports its own tests as not run (no node_modules; Node 20 against a repo requiring 24). The row records the specific overlaps that make them unsafe to apply one at a time: 77cfe12 and 81ffb86 both touch the results-band status derivation and props that merged a7fadf55 also changed; a5d6561 edits the shared Probe fixture four tests depend on; 967e16c could not run Prettier while editing tests/search-results-band-adoption.test.ts, the exact file whose formatting broke Static PR checks and ci/circleci: verify earlier the same day.

#097 — Gitleaks reports a false red when the head moves mid-run. The job triggered for head 9bace1d1 checked out that merge ref, then queried the API and built its range against 40278453, pushed seconds later and absent from the checkout. Git rejected the range, so it scanned ~0 bytes, logged no leaks found in partial scan, and exited 1 — a security gate that did not run, presenting as ordinary noise. It cleared once the head settled (23 commits scanned, ~198 KB). Both range endpoints resolve in any complete checkout, verified locally against the branch and the PR merge ref, so this is not a fetch-depth problem.

#093 updated.main mitigated the symptom in 3a8edb93 by retrying count-and-visibility together in ui-overlap.spec.ts's gotoHome. That hardens one helper; the duplicate root is unchanged. Also records the evidence that it predates PR #1316 — at 631d90d2 the spec already documented "two header#search nodes" and "a second transient header#search can exist briefly".

Verification

  • npm run format:checkAll matched files use Prettier code style!
  • npm run docs:check-linksdocs link check passed: 1297 repo path references resolve.
  • npm run verify:pr-localVerification not run: documentation-only change. No source, test, or config file is touched, so the build, client-bundle scan, unit suite and RAG fixture stages have nothing to exercise. The two checks above are the ones that can fail on this diff.
  • npm run verify:uiUI verification not run: no UI, routing, styling, reduced-motion or forced-colors behaviour changed.
  • npm run eval:retrieval:quality — not applicable; no retrieval, ranking, selection, chunking or scoring behaviour changed.
  • npm run check:production-readiness — not applicable; no clinical workflow, privacy, environment, Supabase, source-governance or deployment behaviour changed.

RAG impact: no retrieval behaviour change — this is a documentation ledger entry. No file under src/lib/rag/**, clinical-search, retrieval-selection, released-search-order, ranking-config, the eval harness, the golden fixture, or the retrieval RPCs is modified.

Risk and rollout

  • Risk: none to runtime. The only failure mode is the ledger itself — a malformed table row would break /issues, so both new rows were written through a script that escapes cell content and the result was re-formatted and link-checked.
  • Rollback: revert the single commit; nothing depends on these rows.
  • Provider or production effects: None. No provider-backed gate was run and no live service was contacted.

Clinical Governance Preflight

Not applicable — this change touches no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output behaviour. It adds two rows and edits one row in a markdown ledger.

Notes

This is memory, not a task list. The value is in #096: seven PRs now target main carrying genuine fixes that have never been executed, and each one's own report reads as green until you look at its testing section. Landing them individually is the risk this row exists to prevent — the recommended path is to consolidate them onto one branch, reconcile the band overlaps, run verify:cheap plus format:check over the combination, and land once.

#097 is deliberately P3. It resolved itself and is not blocking anything. It is recorded because the failure presentation is misleading in a specific way: a red that means "someone pushed" is indistinguishable at a glance from a red that means "a secret was found", and the reflex it trains is to ignore the gate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY


Generated by Claude Code

Three items that would otherwise be lost now that #1316 has landed:
#96 records the seven Codex follow-up PRs whose fixes are real but were
never executed anywhere, along with the specific overlaps that make them
unsafe to apply individually — two touch the results-band status
derivation that the merged a7fadf5 also changed, one edits a shared
test fixture four tests depend on, and one could not run Prettier while
editing the very file whose formatting broke CI twice.
#97 records the Gitleaks push race. It resolved itself when the head
stopped moving, but the failure mode is worth keeping: the scan reported
failure having scanned zero bytes, which reads as noise rather than as a
security gate that did not run.
#93 gains main's mitigation in 3a8edb9 and the evidence that the
duplicate root predates PR #1316 — at 631d90d the spec already
documented two header#search nodes. The helper is hardened; the
underlying duplicate is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
@supabase

supabaseBot commented Jul 28, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)
  • docs/outstanding-issues.md
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c2526c2-4188-406b-bca6-faf1995b5716

📥 Commits

Reviewing files that changed from the base of the PR and between 03d601a and ecb79d2.

📒 Files selected for processing (1)
  • docs/outstanding-issues.md

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@BigSimmoBigSimmo mentioned this pull request Jul 28, 2026
3 tasks
@BigSimmo
BigSimmo marked this pull request as ready for review July 28, 2026 13:17
@BigSimmo
BigSimmo enabled auto-merge (squash) July 28, 2026 13:17

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:65bf018c75

ℹ️ 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".

Comment threaddocs/outstanding-issues.md Outdated
The row cited seven abbreviated SHAs as the thing to consolidate. All
seven are unreachable: git fetch origin <sha> fails for every one, no
open PR or branch carries them, and none was in the squash merge. They
were authored in a sandbox on a branch named work and never pushed, so
the follow-up PRs each commit reported do not exist. A ledger entry
whose only pointer is a dead hash is worse than no entry, because it
reads as tracked.
Rewritten around what is actually true and checkable: the PR #1316
review threads persist and describe each fix with file and line detail,
and two of the findings are still live on main — the unscoped Therapy
Compass retry waiters at use-therapy-data.ts:68, and the adoption gate
returning null for root-path modes at
tests/search-results-band-adoption.test.ts:101, which leaves
/?mode=prescribing and Documents outside the inventory. Both verified
against origin/main rather than inferred.
The rest of the batch had already landed independently and is recorded
as needing no action.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
@BigSimmoClaude

Copy link
Copy Markdown
OwnerAuthor

Re: "Preserve durable refs for the seven follow-up fixes" — correct, and understated. Fixed in 6ee59b72.

I checked rather than assumed, and the situation is worse than "at risk of garbage collection". The commits are already unreachable:

ff5b682: fatal: couldn't find remote ref ff5b682
77cfe12: fatal: couldn't find remote ref 77cfe12
9840ed9: fatal: couldn't find remote ref 9840ed9
81ffb86: fatal: couldn't find remote ref 81ffb86
a5d6561: fatal: couldn't find remote ref a5d6561
967e16c: fatal: couldn't find remote ref 967e16c
e544d0d: fatal: couldn't find remote ref e544d0d

No open PR carries them either — I enumerated all 18 open PRs and none matches. Each of those commits reported "created the required follow-up PR metadata titled …", but no such PR exists. They were authored in a sandbox on a branch named work and never pushed. So recording their full SHAs, as suggested, would not help: there is nothing for a full SHA to resolve to.

The durable record is the review threads on #1316, which persist and describe every fix with file and line detail. The row now points there.

More usefully, I checked which findings are actually still live on main rather than treating the batch as one undifferentiated blob:

FindingState on main
Therapy retry waiters unscoped (use-therapy-data.ts:68)LiveretryWaitersRef is still a bare Array<() => void>
Adoption gate skips root-path modes (search-results-band-adoption.test.ts:101)Live — still return null for pathOnly === "/"
Favourites hub countsFixed — libraryCountsTrusted landed
Document status derivation, 401 session expiry, record-path duplicate noticeFixed and merged

So five of the seven were already resolved by work that did land; two remain, and both are now named with file, line, mechanism, and the shape of the fix, so they can be re-derived without any of the lost commits.

Thank you for this one — a ledger row whose only pointer is a dead hash is worse than no row, because it reads as tracked.

Verification:npm run format:check clean; npm run docs:check-links1298 repo path references resolve; ledger integrity re-checked at 97 rows with #090#097 all present. Documentation-only, one file.


Generated by Claude Code

@BigSimmoClaude

Copy link
Copy Markdown
OwnerAuthor

There is no failing check on this PR — nothing to fix. Flagging before more time goes into it.

I enumerated every completed workflow run on this branch: 30 runs, zero failures. The only non-success is a cancelledPR Policy run on 7f586041, a head superseded twice since (by 6ee59b72, then the main merge d255e3ec).

Current head d255e3ec:

CheckResult
PR policy, Change scope, Sync PR policy bodysuccess
Gitleaks, GitGuardiansuccess
Static PR checks, Semgrep, Semgrep ingestion gatestill running
Build, Unit coverage, Production UI, Migration replay, Container images, Safety and config, Advisory UIskipped — correct for a docs-only diff

A cancelled run renders as a red ✗ in the checks list and is visually indistinguishable from a genuine failure. That is the exact defect recorded in this PR as #095"PR required reports failure for concurrency-cancelled jobs" — and it is why that row asks for cancelled to be treated distinctly from failure in the aggregate. It has now cost a real diagnosis cycle on the very PR that documents it.

How to tell them apart: open the job log. A cancellation shows COVERAGE_RESULT: cancelled / UI_RESULT: cancelled and ##[error]<job> result was cancelled. A genuine failure names a command and its output. For Gitleaks specifically, check the bytes-scanned figure — ~0 bytes with no leaks found in partial scan means the scan never ran (#097), which is a different problem again.

This diff touches one markdown file. The only gates that can legitimately fail on it are format:check and docs:check-links, and both pass locally:

  • prettier --check .All matched files use Prettier code style!
  • npm run docs:check-linksdocs link check passed: 1298 repo path references resolve.

Auto-merge is armed and will take it once the running checks land.


Generated by Claude Code

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:d255e3ec57

ℹ️ 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".

Comment threaddocs/outstanding-issues.md Outdated
Codex pushed back on my own row and was right. retryWaitersRef at
use-therapy-data.ts:68 is genuinely unscoped, but nothing observes the
early settle: useTherapyData lives in the long-lived TcProvider and
requestKey derives only from screen, so it cannot change without the
screen changing. The sole awaiting caller is the band's AsyncButton
inside search-screen.tsx, which unmounts on exactly that transition, and
workspace.tsx uses onClick={b.retryData}, discarding the promise.
I had already rated this below the original Major severity but still
listed it as live with a visible "Retry stops being busy" symptom. That
claim was wrong, and a ledger row asserting a user-visible defect that
cannot occur would have sent follow-up work at unnecessary waiter
keying. The correction is retained rather than deleted, with the
condition that would make it real.
Also links the durable PR #1316 review source, using the repo's pinned
Prettier 3.9.5 rather than a transient npx version.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
@BigSimmoClaude

Copy link
Copy Markdown
OwnerAuthor

Re: "Remove the spurious Therapy Compass retry defect" — you're right, and I've corrected my own row. Fixed in 08acc3fe.

I traced it rather than taking either side on assertion. The decisive detail is that useTherapyData lives in the long-lived TcProvider (bindings.tsx:206) and requestKey derives only from screen:

catalogue: usesCatalogueIndex ? "index" : "full",includePathways: screen==="pathways",includeReference: false,

So requestKey cannot change without screen changing. The sole caller that awaits the promise is the band's AsyncButton at search-screen.tsx:33, which lives inside the search screen and unmounts on exactly that transition; workspace.tsx:36 uses onClick={b.retryData}, discarding the promise, with its busy state coming from b.loading. There is no mounted observer left to see the early settle. Your conclusion holds.

I had already rated this below the original Major severity, but I still listed it as a live defect with a visible "Retry stops being busy" symptom, and that specific claim was wrong. A ledger row asserting a user-visible failure that cannot occur is worse than a missing row — it sends follow-up work at unnecessary waiter keying, which is precisely what you flagged.

The row now:

  • drops it from the live list, so #096 is one gap rather than two — the adoption gate's root-path modes, which I re-verified is still return null for pathOnly === "/" at search-results-band-adoption.test.ts:101
  • retains the correction rather than deleting it, including the condition that would make it real: a future caller awaiting retry() from a control that survives a requestKey change
  • carries your stop rule verbatim — no waiter keying without a reproducer showing a still-mounted control whose busy state clears early

I also took the durable-link idea from your other finding and implemented it here, because 7e3d045 is not reachable — the codex/preserve-followup-fix-references branch was never pushed, same as the seven before it.

One thing worth flagging from that attempt: it reported npm run format:check ❌ — pre-existing formatting issues in 27 files, then formatted the file with npx prettier --write. This repo pins Prettier 3.9.5, and npm run format:check is clean here. That 27-file result is a transient npx Prettier of a different version disagreeing with the pinned one — and writing a file with a mismatched formatter is exactly how format:check broke twice on #1316. I used the repo's own binary.

Verification:npm run format:checkAll matched files use Prettier code style!; npm run docs:check-links1300 repo path references resolve; ledger integrity 97 rows, #090#097 present.


Generated by Claude Code

@BigSimmo
BigSimmo merged commit 6021f6d into mainJul 28, 2026
19 checks passed
@BigSimmo
BigSimmo deleted the claude/top-search-design-mockups-w53znc branch July 28, 2026 14:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BigSimmo@claude