Skip to content

docs(claude): weekly CLAUDE.md refresh 2026-09-22 - #28

Merged
kkroo merged 3 commits into
mainfrom
staff-engineer/docs-claude-weekly-refresh-20260922
Sep 23, 2026
Merged

kkroo merged 3 commits into
mainfrom
staff-engineer/docs-claude-weekly-refresh-20260922

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Sep 22, 2026

Copy link
Copy Markdown

Weekly CLAUDE.md refresh for 2026-09-22 (Paperclip BLO-35290, dispatcher BLO-35205).

Evidence completeness: merged_prs=2 limit=500 (N < LIM, so the merge list is complete, not truncated). Both PRs were read in full — no triage sampling was needed at this volume:

Fixed

  • Helm liveness probe paths (traces to rebase upsteam #27). CLAUDE.md stated "API liveness uses /version, worker liveness uses /metrics". That was true at docs(claude): weekly CLAUDE.md refresh 2026-09-15 #26's merge commit, but rebase upsteam #27 moved both liveness probes to a dedicated /health/live endpoint that performs no database access. Verified directly against the post-rebase tree:

    • helm/hindsight/values.yaml — API livenessProbe.path: /health/live (was /version), worker livenessProbe.path: /health/live (was /metrics)
    • hindsight-api-slim/hindsight_api/api/http.py — /health/live, /health/ready, and /health are now three distinct routes; /health is documented as an alias of /health/ready
    • Replaced the vaguer "reserve /health for explicit database-aware health checks" with the actual three-endpoint contract, plus the chart's own appVersion caveat (older images serve /health only and 404 the liveness probe).

    The readiness guidance is deliberately left unchanged: the chart still points API readiness at /version and worker readiness at /metrics, which is a deliberate local deviation from the API docstring's own /health suggestion, and rebase upsteam #27 did not supersede it.

Added

None.

Pruned

None. Every Blockcast-local block survived the rebase intact and was re-verified against the post-rebase tree rather than assumed:

  • "Keeping Postgres Indexes Usable" — _fetch_with_per_bank_index_plan (4 occurrences in retrieval.py), SET LOCAL plan_cache_mode = force_custom_plan (retrieval.py:90), fetch_unit_dates (ops_postgresql.py), and tests/test_partial_index_plans.py all still present.
  • "Helm Worker Capacity" — still matches values.yaml: WORKER_MAX_SLOTS: "2", WORKER_CONSOLIDATION_MAX_SLOTS: "1", WORKER_RETAIN_MAX_SLOTS: "0", RETAIN_MAX_CONCURRENT: "1".
  • Perf-artifact retention note — unchanged.

Also validated all 49 backtick-quoted file citations and every directory/script reference in CLAUDE.md against a complete git/trees/main?recursive=1 listing (4746 blobs, truncated: false). All resolve. No stale file:line citations or renamed paths.

Uncertain — needs human review

  • configuration.md vs configuration.mdx — deliberately NOT changed. Upstream's CLAUDE.md cites hindsight-docs/docs/developer/configuration.mdx; ours cites configuration.md in three places (lines 130, 481, 533). I checked the actual tree: configuration.md is present on our main and .mdx is absent, so our citation is correct for this fork and upstream's rename has not landed here. Flagging because a future upstream rebase that brings the rename will silently make all three citations stale.
  • .sesskey was added to the repo root by rebase upsteam #27. Outside this PR's scope (root CLAUDE.md only) and I did not investigate it, but a file by that name at the repo root is worth a human glance to confirm it is intended and carries no secret material.
  • Branch name deviates from the runbook. The runbook prescribes docs/claude-weekly-refresh-20260922. That name is structurally unpushable in this repo: a bare branch named docs exists (refs/heads/docs → 4a6942fb), so refs/heads/docs/... is a ref directory/file conflict. Used staff-engineer/, the convention prior refreshes in this repo already follow.

Guardrails honored: no senior-engineer system-prompt block and no Architectural-Principles / Anti-Patterns section was touched. The two Helm sections were edited only because #27 clearly supersedes them. Diff is 8 insertions / 3 deletions in CLAUDE.md alone. Not merged — for human review.

🤖 Generated with Claude Code

PR #27 moved both API and worker liveness probes to the dedicated
/health/live endpoint, which performs no database access. CLAUDE.md
still described the pre-rebase paths (/version and /metrics), which are
now the readiness probes only.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Sep 22, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-35290
🔗 Paperclip issue: BLO-35205

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 948b899

Docs-only change (CLAUDE.md, +8/-3). No executable surface, so the review reduces to whether the prose is factually true of the tree at this head. I verified every claim directly against files fetched at this SHA rather than against the PR description.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

  • [native-codex] CLAUDE.md:191 — "The API serves three monitoring endpoints" is scoped to the API, but the worker serves the same three (worker/main.py:93,104,115) plus /metrics. The preceding sentence already asserts worker liveness uses /health/live, so the paragraph is not wrong — just narrower than the fact it rests on.
    • Consider "The API and worker each serve three monitoring endpoints". Cheap insurance against a future reader seeing /health/live on the worker probe, finding only an API-scoped statement, and "correcting" values.yaml back to /metrics.

Strengths

Claim-by-claim verification at this head — all accurate:

  • helm/hindsight/values.yaml:46,172 — API and worker livenessProbe.path: /health/live. Matches the new text.
  • helm/hindsight/values.yaml:57,183 — API readiness /version, worker readiness /metrics. The retained sentence is still true; correctly left alone.
  • hindsight-api-slim/hindsight_api/api/http.py:5343,5360,5377 — /health, /health/ready, /health/live are three distinct routes. /health and /health/ready share _readiness_response() (:5336), so "supported alias" is literally true rather than aspirational.
  • http.py:5386 / worker/main.py:124 — both liveness handlers return an in-process response and never await health_check(). The "performs no database access" claim is verified in the code, not just in a docstring.
  • helm/hindsight/Chart.yaml:6 — appVersion: "0.10.0", and values.yaml:148 documents tag defaulting to .Values.version then Chart.appVersion. The 404-on-older-images caveat is the right warning and is correctly conditioned on a pinned older tag.

Good judgement on the two things not changed: readiness stays DB-independent even though http.py:5382's own docstring suggests pointing readiness at /health, and the deviation is a deliberate local one that this PR correctly declines to silently "fix". Scope discipline is tight — one file, two adjacent sections, no drift into the guarded sections.

Recommended Action

  1. No blocking changes requested.
  2. Merge once the remaining required CI checks finish green.

@kkroo

kkroo commented Sep 23, 2026

Copy link
Copy Markdown

Lease: updating this branch with main. The red verify-generated-files lane at 948b899 is a baseline drift (memory_engine.py one regenerated line) that every weekly-refresh PR since 2026-09-07 hit and that #29 fixed on main at 21:51Z; this PR only touches CLAUDE.md. A merge from main is the only way to re-run that lane against the fixed main (reruns pin the original merge commit). Ally re-review will spawn from the push.

🤖 Generated with Claude Code

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 89744af

Docs-only (CLAUDE.md, +8/-3). The prose content is unchanged from the previously reviewed head — this sync merged main (#29, recall/torch pool bounds) into the branch, so the review reduces again to whether each assertion is true of the tree at this SHA. I re-verified every claim against files fetched at this head, and this time also against the release history the new compatibility caveat depends on.

Critical Issues (0)

Important Issues (1)

  • [native-codex] CLAUDE.md:198 — "/health/live needs an image at this chart's appVersion or newer — older images serve /health only and would 404 the probe" states the wrong version floor, and the second clause is false for two shipped releases. appVersion is 0.10.0 (helm/hindsight/Chart.yaml:6), but /health/live has shipped since v0.9.1. Verified by content at each release commit rather than by date arithmetic — hindsight_api/liveness.py and the "/health/live" route are absent at v0.9.0 (b12646f49) and present at v0.9.1 (e5b49eb67), v0.9.2 (ebad47824) and v0.10.0 (5d46f9c8c), on both the API (api/http.py) and the worker (worker/main.py). So a v0.9.1 or v0.9.2 image serves all three endpoints, not /health only.
    • The error fails safe — following it never produces a 404 — but it is self-rotting, which is why it is worth fixing now rather than later: "this chart's appVersion" is a moving reference while the true floor is fixed at v0.9.1, so every future release widens the gap without anyone touching the sentence. Pin the constant instead: "/health/live requires a v0.9.1 or newer image; v0.9.0 and earlier serve /health only and would 404 the probe." Worth mirroring into helm/hindsight/values.yaml:42-43,168-169, which carry the same wording and are the actual origin of the claim.

Suggestions (1)

  • [native-codex] CLAUDE.md:195 — "The API serves three monitoring endpoints" is API-scoped, but the worker serves the same three (hindsight_api/worker/main.py:94,105,116) plus /metrics (:138). Raised on the prior head and still open; it now carries a little more weight, because the appVersion caveat at :198 sits inside this API-scoped paragraph while :187 points both probes at /health/live — so the one version warning an operator needs for the worker probe is filed under a heading that disclaims the worker.
    • "The API and worker each serve three monitoring endpoints" fixes both in one edit.

Strengths

Claim-by-claim verification at this head — everything except the version floor is accurate:

  • helm/hindsight/values.yaml:46,172 — API and worker livenessProbe.path: /health/live. Matches :187.
  • helm/hindsight/values.yaml:57,183 — API readiness /version, worker readiness /metrics. The retained sentence at :192-193 is still true and correctly left alone.
  • api/http.py:5351,5367,5386 — /health, /health/ready, /health/live are three distinct routes; /health and /health/ready both delegate to _readiness_response() (:5336), so "supported alias" is literally true rather than aspirational.
  • "performs no database access" is verified in the implementation, not just asserted in a docstring: liveness.py:85-92 and :95-112 build the payload from __version__ and a monotonic clock only, and the worker handler (worker/main.py:124) reads poller attributes in-process. No health_check() on either path.

Good judgement on the two things not changed. Readiness stays DB-independent even though http.py:5382's own description says "point readinessProbe at /health" — a deliberate local deviation this PR correctly declines to silently "fix". Scope discipline is tight: one file, two adjacent sections, no drift into the guarded sections, and the main merge brought no docs conflict.

Recommended Action

  1. Address Important issues this cycle.
  2. Consider Suggestions opportunistically.

Ally Important on PR #28 at 89744af: CLAUDE.md and the two values.yaml
probe comments said /health/live needs an image at this chart appVersion
or newer, with older images serving /health only. The floor is fixed at
v0.9.1 (liveness.py and the /health/live route are absent at 0.9.0
b12646f and present at 0.9.1 e5b49eb, 0.9.2 and 0.10.0 on both the
API and the worker), so the moving appVersion reference overstated the
requirement and would keep drifting on every release.

Controls: grep shows the appVersion phrase gone and the v0.9.1 floor in
all three places; helm lint helm/hindsight passes; stashing values.yaml
brings the old phrase back (negative control).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Omar Ramadan <omar@blockcast.net>
@kkroo

kkroo commented Sep 23, 2026

Copy link
Copy Markdown

Lease: pushing a fix for Ally's Important at 89744af. CLAUDE.md and the two values.yaml probe comments now pin the /health/live image floor to v0.9.1 (v0.9.0 and earlier serve /health only) instead of the moving appVersion reference. Verified: liveness.py and the /health/live route are absent at 0.9.0 (b12646f) and present at 0.9.1 (e5b49eb), 0.9.2 and 0.10.0; helm lint passes; grep positive and stash negative controls ran. Non-force push on top of 89744af.

🤖 Generated with Claude Code

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6c856d9

Docs-only (CLAUDE.md +8/-3, helm/hindsight/values.yaml +4/-4). No executable surface, so the review is again whether each assertion is true of the tree at this SHA. Every claim re-verified against files fetched at this head, and the version floor re-verified by content at each release commit rather than by taking the fix's word for it.

Prior Findings Dispositioned (1)

  • prior:89744af important 1 — fixed — CLAUDE.md:198-199 — the moving appVersion reference is gone; the floor is now pinned to the constant v0.9.1, and the second clause is now literally true rather than merely fail-safe. Independently re-measured at the four release commits: /health/live and liveness.py are absent at v0.9.0 (b12646f49) and present at v0.9.1 (e5b49eb67), v0.9.2 (ebad47824) and v0.10.0 (5d46f9c8c), on both api/http.py and worker/main.py. The mirror I asked for landed too — helm/hindsight/values.yaml:42-43 and :168-169 carry the same pinned wording on the API and worker probes, which is where an operator pinning a tag actually reads it.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

  • [native-codex] CLAUDE.md:195 — "The API serves three monitoring endpoints" is API-scoped, but the worker serves the same three (worker/main.py:94,105,116) plus /metrics (:138). Raised on both prior heads and still open.
    • "The API and worker each serve three monitoring endpoints" is the one-word fix. Weaker than it was, though, and worth saying so: the version caveat that used to be stranded under this API-scoped heading is now also on the worker probe comment in values.yaml:168-169, so the operator path that made this worth escalating is covered. Purely editorial now — fine to leave.

Strengths

The only in "v0.9.0 and earlier serve /health only" is exact, not approximate. At v0.9.0 the API exposes /health and /version and nothing else in that family (api/http.py:4015,4033), and the worker /health and /metrics (worker/main.py:85,99) — /health/ready and /health/live both arrive together at v0.9.1. A looser "serve /health only" would have been the easy thing to write and would have been wrong; this one survives the check.

Claim-by-claim at this head, all accurate:

  • values.yaml:46,172 — API and worker livenessProbe.path: /health/live on ports 8888/8889. Matches CLAUDE.md:187.
  • values.yaml:57,183 — API readiness /version, worker readiness /metrics. The retained sentence at :192-193 is still true and correctly left alone.
  • http.py:5343,5360,5377 — three distinct routes; /health and /health/ready both delegate to _readiness_response() (:5336), which is 200 if health.get("status") == "healthy" else 503. So "supported alias" and "503 when the database is unreachable" are both literally true, on the worker as well (worker/main.py:85-91).
  • "performs no database access" is verified in the implementation, not asserted in a docstring: liveness.py:85-92,95-112 build the payload from __version__ and a monotonic clock only; the worker handler reads poller attributes in-process. No health_check() on either path.

Still correctly not changed: readiness stays DB-independent even though http.py's own route description says "point readinessProbe at /health" — a deliberate local deviation this PR declines to silently "fix". Scope stayed tight across three heads: the fix touched exactly the two files carrying the wrong sentence and nothing else.

Recommended Action

  1. No blocking changes requested.
  2. Merge once the remaining required CI checks finish green.

@kkroo
kkroo merged commit 2962645 into main Sep 23, 2026
99 checks passed
Sign up for free to 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.

1 participant