[FagnerMartinsBrack]

Plan 24, step 1: the foundation package. Nothing imports it yet, so nothing is enforced.

What

• New @packages/crawl-admission:
  • an exact-hostname routing policy (crawl-routing-2026-11-08) with 22 hosts at ten-second start spacing and two seconds for every other host;
  • response classification for 429, 401, LinkedIn 999/403, AO3 418 and the sendibm 451 host;
  • Retry-After parsing: delta-seconds, or an HTTP-date measured against the response Date, plus a 5 s margin. With no valid value it falls back to 60 s, then 600 s for a repeated 429. A valid delay is never shortened;
  • a DynamoDB host-admission provider on the rate-limits table. It takes a conditional lease behind the start-spacing and cooldown checks, releases only for the owner that holds the lease, only ever lengthens a cooldown, and reports a store failure separately from contention.
• CIA reading-room tests in crawl-article for the landing-page rejection that is already on main. They cover the real URL, a valid PDF, the homepage requested on purpose, and every transport leg returning the homepage.

The handoff's appendix patch was applied as written, plus one planned change: the last sentence of the jest.config.js comment is removed, because this package has no c8 ignore annotations.

Verification

• pnpm nx run @packages/crawl-admission:check: 52 tests pass at 100% coverage, and lint and knip pass.
• pnpm nx run @packages/crawl-article:check: 1142 tests pass at 100% coverage.
• The full pnpm check passed in the pre-commit hook.

---

Generated by Claude Code

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(@packages/crawl-admission,@packages/crawl-article): add host admission package #1344
• Commit: e2b76ef

[claude]

Claude finished @FagnerMartinsBrack's task in 1m 5s —— View job

---

I'll analyze this and get back to you.

[FagnerMartinsBrack]

High Priority Issues (Must Fix)

None found

Medium Priority Issues

1. normalizeHostname throws on hostnames that URL accepts

src/packages/crawl-admission/src/routing-policy.ts:3-11

NormalizedHostSchema only accepts /^[a-z0-9-]+(\.[a-z0-9-]+)*$/, and normalizeHostname calls .parse(), so it throws a ZodError when that pattern doesn't match. A WHATWG URL gives back hostnames that don't match it. I checked these in Node:

new URL("https://a_b.example.com/").hostname  // "a_b.example.com"
new URL("http://[::1]/").hostname             // "[::1]"

Hostnames with underscores do turn up in real links, including click-tracker subdomains like the ones on the ten-second list. Nothing imports the package yet, but once the crawl calls normalizeHostname(new URL(url).hostname) before admission, saving such a link will throw instead of being paced with the default policy. CLAUDE.md says .parse() is for cases where invalid data means a bug. Here the hostname is external input, so either widen the pattern to cover everything URL.hostname can return (at least _, and [/]/: for IPv6 literals) or use .safeParse() and give the caller an explicit outcome. Add a test with an underscore host either way.

Fix this →

Low Priority Suggestions

1. Nothing caps the cooldown length

src/packages/crawl-admission/src/retry-after.ts:46-50

Retry-After: 999999999 sets a cooldown of about 31 years. The only cap is LATEST_DATE_MS, the largest valid Date, so a host that sends a broken or hostile header can block itself for good, with nothing to see except the row. The PR says on purpose that "a valid delay is never shortened", so this is a choice and not a bug. Consider logging a cooldown above some size when this gets wired, so it can be noticed.

2. A failed assert in deferralFrom is reported as store-unavailable

src/packages/crawl-admission/src/dynamodb-host-admission.ts:53-56, 108-110

The assert and the ConditionFailedHostAdmissionRow.parse run inside tryAdmit's try. If either fails, the error comes back as { kind: "store-unavailable" }, which mixes an invariant breach (a row that doesn't match the schema, or no gate in the future) with a real outage. This is tested on purpose ("the failed condition leaves no gate in the future"). Still, once callers branch on store-unavailable, for example to fail open, a broken row will quietly look like a DynamoDB outage. You could keep the try around the update call only.

3. The tests check a hand-written copy of the condition expressions

src/packages/crawl-admission/src/dynamodb-host-admission.test.ts:52-105

sharedTable() re-implements the three ConditionExpressions in JavaScript, keyed on UpdateExpression.startsWith(...). The tests prove the provider agrees with that copy, not with DynamoDB, so a typo in the expression string would still pass. A later wiring step could add one integration check against DynamoDB Local or staging.

4. Plan-bound text in runtime data

src/packages/crawl-admission/src/routing-policy.ts:35-36, 69-70

The evidence strings start with "Plan 24…". They are data, not comments, but they will outlast the plan doc. Naming the observation itself ("observed HTTP 429 from this host, week to 2026-11-07") would still make sense after the plan is gone.

5. A caller cannot re-acquire its own lease

src/packages/crawl-admission/src/dynamodb-host-admission.ts:89

A second admitHost call with the same ownerToken while the lease is held is turned away as host-busy against itself. That's fine if callers always release before they admit again, but the types don't say so. It's worth stating in the wiring step.

[FagnerMartinsBrack]

@claude Fix the review issues. Use your Read tool to read the local file .github/workflows/claude-PR-code-review-auto-apply.md for instructions. The file is in the checked-out repository.

Context

• Attempt: 1 of 5
• Repo: Readplace/readplace.com
• PR: feat(@packages/crawl-admission,@packages/crawl-article): add host admission package #1344
• Branch: rp/confident-knuth-0tb34j

Review to Address

See the review comment above this one for the issues to fix.

[claude]

Claude finished @FagnerMartinsBrack's task in 48s —— View job

---

Review issues fixed

• Read the auto-apply instructions and the review
• Medium 1: widened NormalizedHostSchema to accept underscore hosts and IPv6 literals, and added tests
• Ran pnpm nx run @packages/crawl-admission:check
• Committed (the full pnpm check passed in the pre-commit hook) and pushed 6c1f8aa

Change: normalizeHostname used to throw on hostnames that a WHATWG URL returns. It now accepts them: the pattern is ^([a-z0-9_-]+(\.[a-z0-9_-]+)*|\[[0-9a-f:.]+\])$. An underscore host such as a_b.example.com now gets the default policy instead of a ZodError. A value that is not a hostname, like example.com/path, is still refused.

I widened the pattern instead of switching to .safeParse(). Every value URL.hostname can return is a valid host to pace, so callers don't need a separate failure outcome to handle.

New tests: real URL parses for underscore, IPv6, IPv4-mapped IPv6, IPv4 and punycode hosts, plus a check that an underscore host gets the default policy. The package now has 58 tests, all passing at 100% coverage.

Not done: the five low-priority suggestions, which the auto-apply instructions leave out of scope.
· Branch

[claude]

Fixed the medium-priority issue in 6c1f8aa: NormalizedHostSchema now accepts underscore labels and bracketed IPv6 literals, so normalizeHostname(new URL(url).hostname) no longer throws for hosts like a_b.example.com and paces them with the default policy. I widened the pattern rather than switching to .safeParse() because every value URL.hostname can return is a valid host to pace. A value that is not a hostname, such as example.com/path, is still refused. New tests run real URL parses for underscore, IPv6, IPv4-mapped IPv6, IPv4 and punycode hosts. I left the low-priority suggestions alone, as the auto-apply instructions say.

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(@packages/crawl-admission,@packages/crawl-article): add host admission package #1344
• Commit: 6c1f8aa

[claude]

Claude finished @FagnerMartinsBrack's task in 56s —— View job

---

Review of 6c1f8aa

• Read the review instructions
• Read the diff: crawl-admission package (routing policy, Retry-After, DynamoDB admission) and the CIA tests in crawl-article
• Checked the provider against the hutch-rate-limits table (pk hash key, expiresAt TTL) and against writeIfCondition's marshalled oldItem
• Wrote the review

I found no high or medium priority issues. The full review is posted separately by the review workflow.
· Branch

[FagnerMartinsBrack]

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

The fix in 6c1f8aa solves the earlier medium issue. NormalizedHostSchema now accepts underscore labels and bracketed IPv6 literals, and real URL parses test both.

I also checked the DynamoDB provider against the infra and found no mismatch:

• Key and TTL: the key pk and the TTL attribute expiresAt match hutch-rate-limits in projects/hutch/src/infra/hutch-storage.ts:333-341.
• Old item format: ConditionFailedHostAdmissionRow reads the old item as raw { N } / { S } attribute values. That is correct, because writeIfCondition returns ConditionalCheckFailedException.Item, which the document client does not unmarshal.
• Deferral gates: the gates in deferralFrom use the same boundaries as the ConditionExpression: > now on one side and <= now on the other, and the lease is free from leaseExpiresAt + CLOCK_UNCERTAINTY_MS.

Low Priority Suggestions

1. evidence strings still say "Plan 24"

src/packages/crawl-admission/src/routing-policy.ts:35-36, 69-70

The last review raised this and it was left alone. These strings are runtime data that will outlast the plan document. Wording that describes the observation itself would still make sense after the plan is gone, for example "observed HTTP 429 from this host in the week to 2026-11-07".

2. A cooldown write can shorten the row's TTL below a held lease

src/packages/crawl-admission/src/dynamodb-host-admission.ts:146-153

recordHostCooldown sets expiresAt = max(cooldownUntil, now) + 7d. That can be earlier than the max(leaseExpiresAt, nextStartAt) + 7d that tryAdmit wrote, by up to one invocation's length. With seven days of retention this does no harm. If you want the TTL to only ever grow, if_not_exists or a max would do it.

3. Negative mock assertions in the new CIA tests

src/packages/crawl-article/src/crawl-article.test.ts, transport-ladder.test.ts

The new tests use expect(extractPdf).not.toHaveBeenCalled() and expect(h2).not.toHaveBeenCalled(). The existing test just above them uses toHaveBeenCalledTimes(0). The test-driven-design skill prefers asserting what did happen; toHaveBeenCalledTimes(0) would at least match the file's existing style.

4. Earlier low-priority notes still apply

These were raised last time and are still open:

• nothing caps the cooldown length;
• a broken row or a failed invariant is reported as store-unavailable;
• the fake table re-implements the condition expressions instead of testing against DynamoDB;
• a caller cannot re-acquire its own lease.

All four are acceptable for a package that nothing imports yet. They are worth settling in the wiring step.

[FagnerMartinsBrack]

@claude address all low prio issues

[claude]

Claude finished @FagnerMartinsBrack's task in 22m 34s —— View job

---

Low-priority review notes

Pushed in 061fe2f. Of the seven notes, four are fixed and three are left with reasons. @packages/crawl-admission:check passes (59 tests) and @packages/crawl-article:check passes (1142 tests), both at 100% coverage. The full pnpm check passed in the pre-commit hook.

• evidence strings named Plan 24. They now describe the observation itself: "Observed HTTP 429 responses from this host in the week to 2026-11-07…" and "Default for every unlisted host…".
• Negative mock assertions in the CIA tests. The two ladder tests now check which legs ran and how each ended ([["primary", "answered"]]) instead of expect(h2).not.toHaveBeenCalled(). The crawl test uses toHaveBeenCalledTimes(0) to match the test above it. Its calls check already shows what was fetched.
• A broken row looked like store-unavailable. In tryAdmit, the try now covers only the DynamoDB write. When a failed condition returns no row, or a row with no gate in the future, admitHost now throws instead of reporting an outage. The tests were split to match: a thrown store error still gives store-unavailable, and the two broken-row cases now expect admitHost to throw.
• A caller can't re-acquire its own lease. I kept the current behaviour and added a test that pins it, so the wiring step can't miss it. A second admitHost from the same owner, made after the start spacing has passed, is turned away as host-busy until that owner releases. Letting an owner re-enter would change the pacing rules without a caller that needs it.
• No change: a cooldown write can shorten the row's TTL. This has no effect anyone can see. The TTL can come out at most one lease length early, and it fires 7 days after the lease and start-spacing gates have passed. A deleted row and a row whose gates are all 7 days old admit the same way. Making the TTL only grow would take a second conditional write on every 429, and CLAUDE.md's Evidence Over Speculation rule argues against that.
• No change: nothing caps the cooldown length. The PR says on purpose that a valid delay is never shortened, so a cap is a product decision for you, not a fix. The review suggested logging a very long cooldown, but this package has no logger and no caller yet. That belongs in the wiring step, where cooldownAfterRateLimit's result and source reach the crawl's logger.
• No change: the fake table re-implements the condition expressions. The repo has no DynamoDB Local setup, and testing against staging would need a new test target. CLAUDE.md requires your approval for a new command. I suggest adding one integration check against the real hutch-rate-limits table in the wiring step; tell me if you want me to add it here instead.
  · Branch

[FagnerMartinsBrack]

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

• Repo: Readplace/readplace.com
• PR: feat(@packages/crawl-admission,@packages/crawl-article): add host admission package #1344
• Commit: 061fe2f

[claude]

Claude finished @FagnerMartinsBrack's task in 24s —— View job

---

I'll analyze this and get back to you.

[FagnerMartinsBrack]

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

I checked the changes in 061fe2f against the earlier review notes:

• Broken row vs. outage (dynamodb-host-admission.ts:88-118): the try now covers only the writeIfCondition call. A thrown store error still returns store-unavailable. A missing old item or a row with no gate in the future now makes admitHost throw. ConditionalWriteOutcome is exported from @packages/storage-client. The let outcome is definitely assigned because the catch returns.
• The assert holds on every reachable row: deferralFrom uses the same nowMs and the same boundaries as the ConditionExpression. Those are leaseExpiresAt <= now - 5s against leaseExpiresAt + 5s > now, and <= :now against > now for nextStartAt and cooldownUntil. A failed condition therefore always leaves at least one gate in the future. The only row that could break this has ownerToken set and no leaseExpiresAt. No write produces one, because release removes both attributes together.
• Re-entry test: nextEligibleAt is T0 + 315_000, which is the 300 s deadline plus the 10 s lease grace plus 5 s of clock uncertainty. It also shows the owner is admitted again after it releases.
• Ladder tests: they now check which legs ran and how each ended, instead of asserting that a mock was not called. This follows the test-driven-design skill.
• evidence strings: they now describe the observation and no longer name the plan.

Low Priority Suggestions

1. Commit 6c1f8aa has no Conventional Commits scope

fix: address high/medium priority review issues changes only @packages/crawl-admission. The git-commit skill asks for the package scope, as the other two commits on this branch have. If the branch is squash-merged, set the scope on the squash commit's title.

2. Points left for the wiring step

These are unchanged from the last round, and the reasons given for leaving them seem sound:

• nothing caps the cooldown length, and nothing logs a long one;
• recordHostCooldown can shorten the row's TTL by up to one lease length;
• the fake table re-implements the condition expressions instead of testing against DynamoDB.

When the crawl starts calling admitHost, note in its docs or types that a broken row makes it throw. A caller that only checks outcome.kind should not expect store-unavailable for that case.

[FagnerMartinsBrack]

@claude address all low prio issues

[claude]

Claude finished @FagnerMartinsBrack's task in 31s —— View job

---

Low-priority review notes

• Read the trigger comment and the latest review (on 061fe2f)
• Checked each open low-priority note against the branch

The run for this comment already finished on 2026-11-09 and pushed 061fe2f. The review on 061fe2f found no high or medium issues and raised three low-priority notes. This run made no code changes, because none of the three can be handled on this branch without a decision from you:

1. Commit 6c1f8aa has no Conventional Commits scope. Fixing it means rewriting a pushed commit, which needs a rebase and a force push. I can't do either from here. If you squash-merge, use fix(@packages/crawl-admission): … as the squash title. Otherwise the commit stays as it is.

2. Points left for the wiring step. My reasons from 061fe2f still apply:

   • Cooldown cap or logging: a cap would shorten valid delays, which is a product decision for you. This package has no logger and no caller, so the log line belongs where the crawl calls cooldownAfterRateLimit.
   • A cooldown write can shorten the TTL: DynamoDB update expressions have no max. A TTL that only grows would need a second conditional write on every 429, and nothing observed calls for one (CLAUDE.md, Evidence Over Speculation).
   • The fake table re-implements the conditions: the repo has no DynamoDB Local, and testing against the real table needs a new test target. CLAUDE.md requires your approval for a new command.

3. Make it clear that a broken row makes admitHost throw. A test already pins this: dynamodb-host-admission.test.ts:321 uses assert.rejects. Putting it in the docs would mean a new code comment, and the code-comments skill requires you to approve the exact wording before it is written. Here is the wording I suggest, as a doc comment above AdmitHost in host-admission.types.ts:

   /** Rejects, rather than resolving store-unavailable, when the stored row breaks the admission invariant. */

   Reply to approve it, or to change the wording, and I'll add it. You could also leave it for the wiring step.
   · Branch