fix: merged advisory and spec - rate limiter atomicity, branch-age gate and renumber residue - #3604
Conversation
Two defects in the content that reached develop, found by auditing the merged tree rather than the pull request descriptions. The 016 to 018 renumber moved the directory, updated CATALOG.md and rewrote the path references, but left six self-references still naming 016. A grep for "016-claude-cloud-environment" could not find these because they never carried the slug: - tasks.md frontmatter: "Task list for 016 Standardised Claude Code Cloud Environment" - tasks.md T039: run /speckit-analyze "for 016" - spec.md: "How should spec 016 handle this?" - spec.md: "A workflow runs the 016 Jest suites" - spec.md: "contract tests and the spec 016 documentation" - data-model.md: the branch-cleanup rule marked "New (016)" FR-016 and the "16/16 questions" counts in plan.md are requirement and count references, not spec numbers, and are deliberately untouched. The Section 1 table in docs/PLUGIN_ADVISORIES.md was also malformed. Its header and separator declared two columns while all thirteen body rows carry four, so under CommonMark the extra cells are dropped and the status and notes columns never rendered. That silently hid exactly the content three of the four CodeRabbit findings were about, including the filemtime freshness caveat. The second header cell held a link to block-plugin-scaffold, which is a value rather than a column name, and the generator repositories are already tabulated above the table. The header is now Plugin, Native replacement, Status, Notes, with a matching four-column separator. Verified: the spec docs suite passes 37/37, and audit-specs.sh reports "Sequential numbering verified: 001 to 018 with no gaps" with 18/18 naming compliance. semgrep ran 38 rules over 11 files with 0 findings. CodeRabbit review of these changes: 0 findings.
A full CodeRabbit pass over the two merge commits reported four issues, and fixing them surfaced four more on the fixes themselves. All are real. The newsletter example destroyed the form on failure. Its error branch said "Please try again later" while replacing the form with a paragraph, so the address was gone and retry was impossible. The form is now replaced only when the request succeeds; on failure the message goes into an in-place alert region so the address survives. A rejected fetch is caught too, and reports through the same region rather than throwing past it. The rate limiter counted with a get_transient()/set_transient() read-modify-write. Two concurrent submissions could read the same count, both write count + 1, and the limit was silently bypassed, which is the same weakness as the denial of service finding that was previously closed against this sample. Counting now happens in a single atomic statement and the compared value is carried out of that statement by LAST_INSERT_ID(), so it is this request's own count. Three further corrections: a failed counter read now refuses instead of reporting spare capacity, which would have failed open; the comparison is strictly greater than the maximum, so max of 3 permits three requests rather than two; and the docblock describes both behaviours. FR-020 gated auto-deletion on the age of the branch's tip commit. A branch created moments ago can carry an old tip commit, so a fresh working branch was eligible for deletion inside a day of being created. The gate now uses a branch-age signal such as a first-observed timestamp. The categorisation contract's Condition and Configuration rows, the R6 decision record, the data model's cleanup rule, and the two acceptance scenarios were carrying the same tip-commit basis and are aligned, and the research note records why the original basis was rejected. The branch-cleanup contract also deleted a remote branch with a bare `git push origin --delete` after re-checking it, leaving a window in which a push landing between the check and the delete would be discarded. The checked tip OID is now recorded and passed as an explicit --force-with-lease expectation, and the report-only skip is unchanged. The FR-020 test asserted the old "at least 24 hours old" wording, so it is updated to the corrected phrasing and extended with two assertions that pin the branch-age rule, which the previous test did not cover. Verified: the spec docs suite passes 37/37; audit-specs.sh reports sequential numbering 001 to 018 with no gaps and 18/18 naming compliance; semgrep ran 38 rules over 11 files with 0 findings. CodeRabbit review of the merged diff and of each fix: 10 findings, all addressed above.
Two adjustments before this branch is proposed for merge. Spec 009's deletion-candidates.schema.json contracts age_days as "Days since last commit", and the shipped audit derives it from lastCommitDate, so the tip-commit basis behind FR-020 is a deliberate 009 decision rather than an oversight introduced here. That makes the corrected gate an amendment to 009's contract for auto-approved agent branches, not a clarification, and it needs state 009 does not yet keep. Both the FR-020 requirement and the branch-cleanup contract's Configuration row now say so explicitly: the stricter gate is scoped to auto-approved agent branches, the branch-age signal's storage mechanism is recorded as an open decision, and the 009 relationship is named rather than quietly redefined. No behaviour is claimed as settled that is not. Also adds the changelog entry this work needs under Unreleased > Fixed, kept inside the 250-character entry limit the validator enforces, and re-checked against it: 145 entries with 10 failures, the same 10 develop already carries, so nothing new is introduced.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No description provided. |
📋 Changelog Quality Validation
Status✅ Validation PASSED - No new failures introduced by this PR. No action required. |
github-actions
Bot
added
priority:normal
area:documentation
lang:js
area:testing
area:jest
and removed
priority:important
labels
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lightspeedwp/.github/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request updates spec 018’s branch-cleanup requirements and the newsletter advisory example. Cleanup eligibility now depends on continuous branch observability, and scheduled deletion checks the branch tip. The newsletter example preserves the form after failed requests and uses database counters with transactional rate-limit checks. ChangesBranch cleanup requirements
Newsletter advisory examples
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other · Severity of issue fixed: Low Merge Risk: 🟡 Moderate · up to The changes are documentation and example code, but readers who copy the newsletter example would report database failures as rate limits. Those users would be told to wait an hour. The branch-cleanup spec also gives an inconsistent outcome for some deferred candidates. Clarify both before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The revised newsletter example improves concurrent counting, but its failure handling does not fully protect the promised rollback behavior if database transaction commands fail. The change is published guidance rather than a demonstrated production deployment, so the operational impact is uncertain. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (8 skipped: 8 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
Merged as a commit rather than a rebase, and the branch was fetched immediately beforehand because the automation on it has force-pushed before. The only failure on this branch was the develop-current rule, which fails because develop has moved, so this merge is the whole fix. The Claude guard contract tests already passed before the merge and still do. CHANGELOG.md auto-merged as a union with no conflict markers and no lost entries, going from 406 entries on this branch and 407 on develop to 408 merged. This merge deliberately does not bring in the branch-age correction to FR-020, which is still open on #3604. This branch does not reference FR-020, the age wording or AUTO_DELETE_MIN_AGE anywhere, so it neither needs that correction to go green nor conflicts with it. But it does implement spec 018's auto-delete rule, so if the branch-age question is decided in favour of a first-observed signal, this branch's implementation of that rule will need revisiting. Verified: the two guard test suites pass 99/99; audit-specs.sh reports sequential numbering 001 to 018 with no gaps and 18/18 naming compliance; semgrep ran 120 rules over 27 files with 0 findings. CodeRabbit could not review this merge: the quota is exhausted (0 of 3, resets 2026-10-01). The verification above was used in its place.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Dismissed on evidence, not on a CodeRabbit re-approval. All three of these reviews predate the current head dd6960d25, and CodeRabbit has not completed a review at this head, so this is not an approval being laundered into a dismissal.
Reviews dismissed:
| Review ID | Commit reviewed | Submitted |
|---|---|---|
| 5331284027 | 74b1b58a7 |
2026-09-27T17:11:44Z |
| 5331506590 | 69b298692 |
2026-09-27T18:23:43Z |
| 5343214679 | 6efc9c7eb |
2026-09-28T18:50:51Z |
Every finding they raised, and where it is addressed:
| Finding | Addressed in | Evidence at dd6960d25 |
|---|---|---|
data-model.md:59 lifecycle diagram still tip >= 24 h old |
9581f44d47 |
reads 009 categorisation; auto-approved deletion is deferred until a branch-age signal exists |
| FR-020 branch-age gate with no defined signal | 6cbdee8a73 |
FR-020, the contract, the data model, research.md and scenario 5 all state the deferral explicitly |
Scenario says more than 24 hours vs FR-020 at least |
f6cd30fba6 |
3 occurrences of at least 24 hours, 0 of more than 24 hours |
| Distinguish 429 from 5xx in the newsletter handler | f6cd30fba6 |
response.status === 429 handled distinctly; address-validation and provider failure give separate advice |
| Rate-limit rows need expiry cleanup | babcb0765c |
DELETE FROM ... WHERE window_started <= %d with KEY window_started; fails closed on purge error |
| Contract must not promise draft-PR approval for every candidate | f6cd30fba6 |
contract states no route from DISCUSS to draft-PR approval |
| 429 message blames the address field | f6cd30fba6 |
Too many attempts. Please try again later. — neutral, because the response does not say which counter fired |
| Storage failure must be distinct from a limit refusal | f6cd30fba6 |
limiter returns 'unavailable'; caller maps it to 503, genuine refusals stay 429 |
START TRANSACTION result unchecked; ROLLBACK result discarded |
f6cd30fba6 |
both checked; a failed discard is logged, not swallowed |
| IP counter charged before the email check, no refund | 06a05a0ee8 |
both counters inside one transaction, email charged first, refusal rolls back both |
| Docstring coverage 25% | not actioned | the analysed functions are PHP inside a Markdown fenced example, not shipped code |
One claim in these reviews was itself wrong, and I have corrected it rather than accepting it. Review 5331284027 and the walkthrough both credit this PR with fixing a malformed Section 1 table header. That defect does not exist: docs/PLUGIN_ADVISORIES.md as merged by #3387 parses as 4 header cells, 4 delimiter cells and 13 four-cell rows, so no column was ever dropped. The changelog entry and body claiming a rendering fix are corrected in dd6960d25, and FEEDBACK_RESPONSE.md item 17 records the withdrawal as Rejected.
Current state at dd6960d25: MERGEABLE, develop is an ancestor so no merge is outstanding, 0 failing checks, Jest 62/62, spec audit 18/18, semgrep 0 findings, and validateAIFeedback returns passed: true where it previously failed.
Still outstanding and not claimed as resolved: no CodeRabbit review has completed at this head. Its CLI is rate limited and I have stopped polling it per instruction; the GitHub app reviews pushes on its own and I will respond to whatever it posts.
PR Template RoutingBranch Type: This PR was automatically routed based on the branch naming strategy. |
These came from the pull request review rather than the local uncommitted pass, which is why the previous report called the branch clean while the review state still had them open. All three were real. FR-020 required a branch-age signal that nothing provides. Spec 018's data model states there is no persistent storage, and spec 009 supplies only age_days from last_commit_date, so the 24-hour auto-delete gate had no usable signal and the requirement was unsatisfiable as written. The auto-approved deletion is now explicitly deferred: until the storage and retention mechanism for a branch-age signal is decided and built, no claude/* branch qualifies for automatic deletion and every candidate follows spec 009's categorisation and draft-PR approval. Deferring is the safe direction, because spec 009's tip-commit basis would otherwise let a branch created moments ago be deleted inside a day of being created. FR-020, the contract's Configuration row and auto-delete step, the R6 decision record and Acceptance Scenario 5 all now say so. Acceptance Scenario 5 required "more than 24 hours" where FR-020 says "at least", so a branch observed for exactly 24 hours satisfied the requirement and failed the scenario. That inconsistency was introduced when the scenario was rewritten to use the branch-age basis. The scenario now matches FR-020. The newsletter handler gave one message for every failure, including the 429 the route returns when the rate limit is hit. Telling someone to check an address that is already valid, while they are blocked, only invites a retry that will be refused again. The handler now separates a rate-limited address, a rejected address, and a provider-side failure. Two test lookups followed the contract's auto-delete row, which is now labelled deferred rather than new, so they were updated to match. Verified: the spec docs suite passes 37/37; audit-specs.sh reports sequential numbering 001 to 018 with no gaps and 18/18 naming compliance; the changelog validator reports the same failure count as develop, with the entry reworded to 250 characters to describe the deferral accurately; semgrep ran 119 rules over 12 files with 0 findings. CodeRabbit review of these changes: 2 further findings raised and addressed, final pass clean.
|
@coderabbitai full review |
|
…y-and-spec-corrections
…spec The counter write is now a single multi-row upsert, so a dropped connection cannot replay one key's increment outside the transaction where a later ROLLBACK cannot undo it. A read of the resulting counts is a second statement, so the connection handle is compared before the counts are believed: a replaced handle yields a storage fault, which the caller answers 503, rather than a request allowed but never charged. The upsert resets an expired window's count instead of incrementing it, so a key the purge has not reached stops being blocked after its window ends, and carries the new value out through LAST_INSERT_ID on both branches. The single-key builder gains the same reset so copying it is safe. The spec records the deferral consistently: FR-020's workflow clause and SC-002 no longer promise automatic deletion that is deferred, the audit/deletion edge case names the placeholder re-check and the tip lease the contract requires, and the deferral's two blockers, its consequences and the items it makes safe to land are stated once consistently.
|
@coderabbitai review |
|
The changelog entry and the pull request body both stated that the Section 1 advisories table had a two-column header while its rows carried four, so CommonMark dropped the Status and Notes columns. That defect is not present in the content merged by #3387. docs/PLUGIN_ADVISORIES.md parses cell by cell as four header cells, four delimiter cells and thirteen four-cell rows, with the header and delimiter counts equal, so no column is ever dropped. The only change this pull request makes to that table is relabelling Caveat as Notes plus re-padding to the compact form used elsewhere in the file. The changelog entry is rewritten to describe what actually changed, and keeps the three real behaviour changes: the newsletter form surviving a failed request, a blocked address being distinguished from a server fault, and atomic rate-limit counting. FEEDBACK_RESPONSE.md records the withdrawn claim as item 17 with the evidence, and the summary no longer asserts that nothing was rejected. No shipped behaviour moves. The change is documentation only, and no test asserted the false claim, so none needed updating; the table shape was verified directly instead.
eleshar
left a comment
eleshar
left a comment
There was a problem hiding this comment.
Table-header claim withdrawn as unfounded — dd6960d25
Part A relevance triage found one claim in this pull request that is false, so I corrected it rather than letting it stand. Documentation only; no shipped behaviour moves.
The claim, and why it is false
Both the changelog entry and the body claimed the Section 1 advisories table had a two-column header while its rows carried four, so CommonMark dropped the Status and Notes columns.
I parsed docs/PLUGIN_ADVISORIES.md as merged by #3387 cell by cell, honouring backtick spans so a pipe inside `code` is not treated as a cell boundary:
| Row | Cells |
|---|---|
| Header | 4 — Plugin, Native replacement, Status, Caveat |
| Delimiter | 4 |
| Data rows | 13, every one with 4 |
Header count equals delimiter count, so the GFM table rule never drops a cell. No column was ever dropped. The only change this PR makes to that table is relabelling Caveat as Notes plus re-padding to the compact form used elsewhere in the file; all 13 data rows are byte-identical to develop.
What changed
- CHANGELOG.md now describes what actually changed, and keeps the three real behaviour changes: the form surviving a failed request, a blocked address being distinguished from a server fault, and atomic rate-limit counting. 238 characters against the validator's 250 limit.
- FEEDBACK_RESPONSE.md records the withdrawn claim as item 17, marked
Rejected, with the parse counts as the evidence, and the summary no longer asserts that nothing was rejected.
A mistake worth flagging
My first draft dropped "tells a blocked address from a server fault" from the changelog to fit 250 characters. That is a real behaviour change in this PR, not a wording detail: PLUGIN_ADVISORIES.md:111,115,118 give distinct advice for a rate limit, a rejected address and a provider failure. A reviewer caught it and I restored it.
Separately, a repo-wide prettier --write I ran reformatted 3,366 unrelated files. I caught it at staging, reverted every one, and the commit is 2 files.
Verification
| Check | Result |
|---|---|
| Table structure parsed cell by cell | 4 / 4 / 13, header equals delimiter |
| Claim guard (new) | Fails on f88321a63a, passes on this head |
| Jest | 62 passed (2 suites) |
audit-specs.sh |
18/18, sequential 001–018 |
| prettier / markdownlint | clean / 0 issues |
semgrep p/security-audit + p/secrets |
0 findings |
validateAIFeedback |
passed: true (was failing) |
CHANGELOG.md still fails prettier --check, but that is pre-existing across 83 historical rows and I did not touch them.
Relevance triage against develop (aabd241)
| Claimed fix | State on develop | Verdict |
|---|---|---|
| Rate limiter atomicity + InnoDB | get_transient/set_transient still at PLUGIN_ADVISORIES.md:123,131 |
needed |
| Branch-age gate | research.md:91 and FR-020 still say tip-commit age |
needed |
Six stale 016 references |
all 6 still present (tasks.md:2,233, data-model.md:73, spec.md:39,40,209) |
needed, count exact |
| Table header | already well-formed at 4 columns | claim withdrawn |
The branch is already current with develop: develop is an ancestor of the head, so no merge was needed.
Two premises in the earlier walkthrough I could not confirm and am not asserting: there is no NESTED_DEPTH and no GraphQL-mutation logic anywhere in #3524's guard, and its new spec text is a stated limitation that sh -c/eval are out of scope.
#3524 is open and unmerged; the body line saying otherwise has been corrected in FEEDBACK_RESPONSE.md. #3558 is merged, so the body note describing it as outstanding work no longer applies.
|
All three items verified against the current head 1. "AI Feedback Validation Report is still failing" — now genuinely fixed. This was a real failure, and my first attempt made it worse. The validator ( Committed wording: 2. 3. "CodeRabbit has not reviewed the head" — still outstanding, and I am not claiming otherwise. No CodeRabbit review has completed at No develop merge was needed. Correction to this PR's own claims. The changelog and body said the Section 1 advisories table had a two-column header dropping the |
|
Resolving this walkthrough on evidence, not on a CodeRabbit re-approval — no fresh CodeRabbit review has completed at The walkthrough carries one retained security concern worth stating explicitly, because it is the only substantive item in the thread and I am resolving rather than ignoring it:
Addressed in
The walkthrough's own two merge-risk items are the same defect seen from the caller side: "readers who copy the newsletter example would report database failures as rate limits" and "the branch-cleanup spec gives an inconsistent outcome for some deferred candidates". Both are covered above and by the contract's Two things I am explicitly not claiming: the stub tests cannot establish real concurrent isolation against a live MySQL, and this PR does not demonstrate a production deployment. That limitation is recorded in the pull request body under "Not tested". The walkthrough also states "changes the replacement table's final column label to 'Notes'" — that part is accurate. Its title check credits the PR with a "table-header correction", which is not: the merged table was already well-formed, and the claim is withdrawn in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
printf 'HEAD '; git rev-parse --short HEAD
printf 'Named files:\n'
fd -a '^(ai-feedback-helpers\.cjs|FEEDBACK_RESPONSE\.md|data-model\.md|PLUGIN_ADVISORIES\.md|CHANGELOG\.md)$' . | head -35
printf 'Validator outline:\n'
ast-grep outline scripts/validation/ai-feedback-helpers.cjs --match 'validateAIFeedback|checkDeferredWithoutIssue' 2>&1 | head -90
printf 'Validator matching lines:\n'
rg -n -C4 'feedback\\s|feedback items|checkDeferredWithoutIssue|Refs|issue' scripts/validation/ai-feedback-helpers.cjs | head -120
printf 'Feedback lines:\n'
rg -n -C3 'All feedback items addressed|## Deferred|3358|1396|Rejected|Section 1|17' FEEDBACK_RESPONSE.md | head -180
printf 'Lifecycle:\n'
sed -n '50,65p' .github/specs/018-claude-cloud-environment/data-model.md
printf 'Table header:\n'
rg -n -C3 'Status.*Notes|Notes.*Status|Section 1' docs/PLUGIN_ADVISORIES.md | head -90
printf 'Changelog claims:\n'
rg -n -C2 'Section 1|two.column|header|advisories' CHANGELOG.md | head -90
printf 'Develop ancestry:\n'
git merge-base --is-ancestor origin/develop HEAD; printf 'ancestor_exit=%s\n' "$?"
printf 'PR head and reviews:\n'
gh pr view 3604 --repo lightspeedwp/.github --json headRefOid,latestReviews,mergeable,mergeStateStatus --jq '{headRefOid,mergeable,mergeStateStatus,latestReviews:[.latestReviews[]?|{state,submittedAt,commit:(.commit.oid // null)}]}' 2>&1 | head -90Length of output: 41504 🏁 Script executed: #!/bin/bash
set -uo pipefail
printf 'Section 1 header on develop:\n'
git show origin/develop:docs/PLUGIN_ADVISORIES.md | sed -n '32,39p'
printf 'PR body issue reference:\n'
gh pr view 3604 --repo lightspeedwp/.github --json body --jq '.body' | rg -n -C1 'Refs #1396|Resolves #1396|Closes #1396|Fixes #1396' | head -35
printf 'Recent reviews and their commit IDs:\n'
gh api repos/lightspeedwp/.github/pulls/3604/reviews --jq '.[-5:] | map({state, submitted_at, commit_id})' 2>&1 | head -60Length of output: 3597 🏁 Script executed: #!/bin/bash
set -uo pipefail
gh api repos/lightspeedwp/.github/pulls/3604/reviews \
--jq '[.[] | select(.commit_id == "dd6960d25b20bd284f2f8d1a3bbf9736ff859ac1") | {user:.user.login,state,submitted_at,body,html_url,commit_id}]'Length of output: 4533
You are also right about the table. The version on GitHub lists a review at ✏️ Learnings added
🧠 Learnings usedYou are interacting with an AI system. |
eleshar
changed the title
|
@CodeRabbit full review |
|
#3604 landed on develop and rewrote FEEDBACK_RESPONSE.md, so the two branches conflict on it. That file is a single shared path for whichever pull request most recently wrote it, which is the collision already recorded in #3618, so this merge is the second pull request to hit it. Our record is kept here, since this branch is #3524 and #3604's own record is already in develop's history. Nothing from #3604 is lost by that: the file records one pull request at a time, and theirs is committed on develop. The findings from the independent adversarial review of the GraphQL fix are recorded in the response file. Three were regressions the change introduced and are fixed in af0c67a, each with a test that fails before. The six that were pre-existing are stated limits rather than fixes: this pull request has to converge, and each new guard capability has drawn new findings. They are written into the hooks contract and the cloud environment doc, and they need a follow-up issue rather than another commit here.
|
Retrospective gate record — tracked in #3688. What was missing at merge time. This pull request merged without a Retroactive CodeRabbit review result. Re-run on the merged diff with
Both were verified against current Retroactive outcome. Fixed in #3689. The changelog entry, the remaining Assumptions and success-criterion wording, and a third finding from #3682's merge are carried there too. This pull request is not reverted. Not fixed, deliberately. Two residual inconsistencies in the same spec remain and are recorded in #3688 rather than changed here: |
1 participant
Linked issues
Refs #1396
Relates to #1396, #3525 and #3524.
This pull request closes nothing. The
Refslink is deliberate: the repository's ai-feedback validation requires aResolves,Closes,FixesorRefsissue reference, and none of the three issues is resolved by this change. #1396 in particular still needs its acceptance criterion confirmed before it can close. #1396 stays open pending your confirmation of the reworded AC3 below, and #3525 and #3524 are already merged or are yours to land.Context
docs/PLUGIN_ADVISORIES.mdanddocs/PLUGIN_REGISTER.md(merged via docs: add versioned plugin advisories for issue 1396 #3387) and the spec 018 documents (merged via docs(specs): spec 016 standardised Claude Code cloud environment #3525). Documentation and specification only — no shipped code changes.Reproduction
count + 1, and both are allowed. Reproduce by firing simultaneous requests at one address and observing fewer stored hits than requests sent.claude/*branch from an old commit, wait for the scheduled audit, and it is reported auto-approved for deletion despite being seconds old.git push origin --delete; a push landing in between is discarded.016and six self-references match, none of which carry the directory slug.Root Cause
Three findings, two of them real causes that were not caught before merge:
CHANGES_REQUESTEDfrom 2026-09-24. A full review of the merged diff then found four real defects.CATALOG.mdand rewrote path references, but a search for016-claude-cloud-environmentcannot find bare self-references that never carried the slug.Fix Summary
What was wrong, and what changed. The tables below are the substance; the narrative is why each one is a defect rather than a preference.
From the review of the merged diff
scripts/cleanup-branches.js:317computesdaysSince(lastCommitDate)git push origin --deletescripts/cleanup-branches.js:342get_transient/set_transientread-modify-writecount + 1, and the limit is bypassed — the same weakness as the denial-of-service finding previously closed against this sampleLAST_INSERT_ID()Found while fixing the above
>= $maxin the rate limitermax: 3it blocked the third request, so only two got throughmax: 3permits threefetch()rejections were not caughthiddenstayed set, so the message was in the DOM and invisiblealert.hidden = falsebefore setting the textresearch.mdanddata-model.mdstill carried the tip-commit basis"at least 24 hours old", so correcting the spec broke the guard rather than the guard catching the changeThe two you asked to fold in
Six stale
016self-references in the spec 018 documents, which the renumber missed because they never carried the directory slug: thetasks.mdfrontmatter description, task T039, three inspec.md(the 009-exception clarification, the Jest suites reference, and the documentation contract), and theNew (016)rule indata-model.md.FR-016and the "16/16 questions" counts are requirement and count references, not spec numbers, and were deliberately left alone.The Section 1 table header: withdrawn. An earlier version of this pull request claimed the header and separator declared two columns while the rows carried four. That was wrong. A cell-by-cell parse of the merged file gives a four-cell header, a four-cell separator and thirteen four-cell rows, so nothing was hidden. The only change to that header is cosmetic (compact spacing and the last column's label). The claim was withdrawn in
dd6960d25and is recorded in the CHANGELOG entry andFEEDBACK_RESPONSE.mditem 17.Alignment check — one item needs your decision
You asked me to check the two spec 018 fixes against your stated direction rather than assume mine was the only reading. On the
--force-with-leasefix I am confident, and I verified it rather than trusting the documentation:That is git 2.43.0 against a real bare remote, and it confirms the option applies to
--deleteand does what the contract now says.The branch-age gate is the one I am not settling for you. Spec 009's
deletion-candidates.schema.jsoncontractsage_daysas "Days since last commit", and the shipped audit derives it fromlastCommitDate. So the tip-commit basis is a deliberate 009 decision, not an oversight — which means my fix is an amendment to 009's contract for auto-approved agent branches, not a clarification, and it needs state 009 does not currently keep. The options are:age_daysmeaning for this rule and persist a first-observed timestamp per branch. Correct, but adds state and an amendment to a merged spec.I have written the requirement and the contract so they state the safety constraint and name this as an open decision, rather than quietly redefining 009 or asserting a storage design you have not chosen. If you prefer option 2 or 3, the wording needs a small follow-up, and the two new test assertions would need to move with it.
Verification
Automated
npx jest --config .jest.config.cjs tests/js/claude-cloud-environment-docs.test.js— 37/37 passbash .specify/scripts/bash/audit-specs.sh— "Sequential numbering verified: 001 to 018 with no gaps", 18/18 naming compliance, 18/18 spec.md presentdevelopalready carries, so nothing new is introduced. The entry is held under the 250-character limit the validator enforces.npm run validate:branch-name—fix/merged-advisory-and-spec-correctionsis valid against the canonical 38-type list. The branch was originally pushed asaudit-3387-3525, which fails the repo's own validator for lacking a{type}/prefix, and was renamed before this PR.semgrep scan --config p/security-audit --config p/secrets --config p/typescript— 38 rules, 12 files, 0 findingscoderabbit review --agentbefore each of the three commits: 10 findings raised across the merged diff and the fixes, all addressed, and the final pass on this branch is cleanqodo review --base origin/develop— could not run:AGENT-QUOTA-EXCEEDED, the organisation is out of Qodo credits. Proceeding without it rather than blocking, as agreed.Behaviour
The
--force-with-leaseclaim is verified against a real bare remote rather than taken from documentation, because the whole fix rests on the option applying to--delete:Regression
The only executable code changed is inside fenced examples in a Markdown document, so no shipped behaviour moves. The one test that guards this spec was failing after the FR-020 correction and was updated with the wording, plus two new assertions pinning the branch-age rule, so the guard is now stronger than before rather than merely relaxed. The section 018 test suite, the numbering audit and the changelog validator are the areas most likely to be affected by a renumber, and all three pass.
Changelog
One entry under Unreleased > Fixed, titled Merged Advisory and Spec Defects Corrected, carrying the
meta:needs-changeloglabel from thefix/*template. Held to 249 characters against the validator's 250 limit and re-checked against the banned-keyword list, so it introduces no new critical.Not tested
The rate limiter and newsletter handler are documentation examples, so their runtime behaviour was not executed. The PHP relies on a
{$wpdb->prefix}ls_rate_limitstable that the example now documents but does not create in code, and MySQL'sLAST_INSERT_ID(expr)behaviour insideON DUPLICATE KEY UPDATEwas reasoned from its documented semantics rather than run against MySQL. The spec 018 branch-age and lease rules are specification text; the shipped audit that would implement them is not ondevelopyet, so only thegit pushhalf was empirically verified.Validation
npx jest --config .jest.config.cjs tests/js/claude-cloud-environment-docs.test.js— 37/37 passbash .specify/scripts/bash/audit-specs.sh— "Sequential numbering verified: 001 to 018 with no gaps", 18/18 naming compliance, 18/18 spec.md presentdevelopalready carries, so no new failure introducednpm run validate:branch-name— branch isfix/merged-advisory-and-spec-corrections, valid against the canonical 38-type listsemgrep scan --config p/security-audit --config p/secrets --config p/typescript— 38 rules, 12 files, 0 findingscoderabbit review --agentrun before each of the three commits; 10 findings raised in total across the merged diff and the fixes, all addressed above, and the final pass on this branch is cleanRisk & Rollback
scripts/validation/validate-branch-name.cjsacceptsrelease/v1.2.3;lib/validate-branch-name.jsandscripts/validation/validate-branch-name.jsreject it; the pr-agent skill copy returned an empty object for every input I gave it, so I am not claiming it is broken — it needs a look. fix: branch-validator - accept semver release branches #3558 coverslib/validate-branch-name.jsonly. Spec 018's contract already removes thescripts/lib/copy, so this is a pre-existing repo inconsistency rather than something spec 018 introduced.Summary by CodeRabbit
Bug Fixes
Changes
claude/*branches is deferred until branch age can be measured. Candidates follow the normal categorization and draft-PR approval process instead.