Harden durable evidence replay edge cases

[justin808]

Summary

• reject malformed https: evidence references even when a valid URL appears beside them
• accept structurally valid consumer-defined runtime/user metric names and units instead of enforcing a non-portable semantic vocabulary
• retain fail-closed source, measurement-pair, duplicate-name, placeholder, size, and byte-memory safeguards

Follow-up to #262. Carries over and resolves the two late P2 findings that arrived after its merge.

Validation

• focused replay suite — 103 runs, 1,205 assertions, pass
• rubocop skills/post-merge-audit/bin/closeout-evidence-replay skills/post-merge-audit/bin/closeout-evidence-replay-test.rb — pass
• git diff --check — pass
• bin/validate — pass
• independent exact-head checker at ad14ebb1b2a376e72e8628791e4f66d38cace99c — clean

Codex Decision Log

• Non-blocking: replace the metric-name allowlist with another vocabulary or defer semantic classification to the consumer seam
  • Decision: validate portable structure and defer semantic truth to the named stable consumer seam
  • Why: the shared pack cannot distinguish a legitimate consumer-defined metric such as checkout_error_count from an arbitrary label without repository-specific knowledge; the contract already states that replay validates shape rather than truth
  • Review later: None
• Non-blocking: malformed repository-seam sources such as source=https:broken predate this follow-up
  • Decision: defer source-reference liveness and shape hardening to separate scoped work
  • Why: this PR fixes the two carried findings without silently expanding the source-reference contract; the replay helper validates portable evidence structure, not source liveness or authorization
  • Review later: audit repository-seam source validation independently

QA Evidence

• QA lane: /root/pr262_test_audit_resume; independent read-only exact-head checker
• Scope checked: durable URL parsing and measured-metric portability in closeout-evidence-replay
• Tested at: candidate head ad14ebb1b2a376e72e8628791e4f66d38cace99c
• Automated checks: focused replay suite; RuboCop; diff check; full repository validation
• Manual checks: parent/head probes for malformed HTTPS beside a valid URL, labeled no-space HTTPS field delimiters and semicolon URL content, exact URL diagnostics, consumer-defined metrics, terminal/interior/prefixed size and structural-count shapes, compound runtime names, exact/punctuation/fused unresolved sentinels, bounded legitimate domain vocabulary, alias units, and snake/camel unresolved forms
• Findings: implementation fixes both carried P2 findings; independent checker found no blocker
• QA required: yes
• QA required rationale: this changes a developer-workflow merge gate and needs independent exact-head adversarial verification
• QA lane status: satisfied
• Release-blocking status: clear
• Process-gap disposition: script

Confidence note: independent exact-head audit is clean; hosted checks and current-head automated reviews remain pending.

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fb72e5ca-b020-4478-be7b-316245067aca

📥 Commits

Reviewing files that changed from the base of the PR and between 3fc5d3d and 2f91899.

📒 Files selected for processing (3)

• CHANGELOG.md
• skills/post-merge-audit/bin/closeout-evidence-replay
• skills/post-merge-audit/bin/closeout-evidence-replay-test.rb

Walkthrough

The PR updates qa-evidence v2 validation to detect malformed HTTPS references, accept structurally valid consumer-defined metrics, reject placeholder- and size-related metrics, and distinguish structural counts from runtime measurements. Replay tests and the changelog document these rules.

Changes

qa-evidence v2 validation

Layer / File(s)	Summary
HTTPS reference validation skills/post-merge-audit/bin/closeout-evidence-replay, skills/post-merge-audit/bin/closeout-evidence-replay-test.rb	Shared HTTPS token extraction now handles malformed references consistently, with replay coverage for visual, interaction, repository-seam, and delimiter cases.
Measured metric validation skills/post-merge-audit/bin/closeout-evidence-replay, skills/post-merge-audit/bin/closeout-evidence-replay-test.rb	Measured metrics use camel-case-aware tokenization and reject unresolved, size-shaped, and structural-count names while accepting consumer-defined runtime metrics and preserving byte-memory checks.
Replay documentation CHANGELOG.md	The unreleased changelog records the expanded metric portability rules and malformed HTTPS rejection behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• shakacode/agent-workflows#262: Related qa-evidence v2 changes to measured-metric validation and HTTPS URL extraction.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the PR’s main theme of hardening durable evidence replay edge cases, including malformed URLs and metric validation.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/pr262-post-merge-hardening

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/post-merge-audit/bin/closeout-evidence-replay`:
- Around line 571-576: Update the placeholder-name rejection near
UNRESOLVED_PLACEHOLDER to use token-aware matching that detects placeholder
terms within snake_case names, while preserving the existing exact-name checks
and size-metric exclusions. Add regression coverage for compound names such as
unknown_metric, missing_value, and todo_count to ensure they cannot produce
SATISFIED results.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 219a8b31-fd36-474c-8b59-794dc41c0e90

📥 Commits

Reviewing files that changed from the base of the PR and between 162e168 and 4ec23a9.

📒 Files selected for processing (3)

• CHANGELOG.md
• skills/post-merge-audit/bin/closeout-evidence-replay
• skills/post-merge-audit/bin/closeout-evidence-replay-test.rb

[claude]

Review summary

What this PR does: follow-up hardening to closeout-evidence-replay (the QA-evidence merge-gate validator):

1. Rejects malformed https: references even when a valid URL appears alongside them, by widening http_urls to capture any https:... fragment (not just well-formed https://...) so valid_https_url? can reject the malformed one instead of silently ignoring it.
2. Drops the closed runtime_tokens allowlist that previously required measured-metric names to contain a recognizable web-vital/runtime word, replacing it with a narrower "not an exact placeholder word / not a size word" check — intended to let consumer repos use their own metric vocabulary (e.g. checkout_error_count).

Fix #1 (malformed HTTPS detection) — looks correct

http_urls now matches https:[^\s<>()]* instead of requiring https://, so a malformed fragment like https:broken is captured and fails valid_https_url? (via URI.parse + URI::Error rescue), flipping http_url?/verdict to fail-closed as intended. without_https_urls (used to strip URLs before scanning for metric names/measurements) intentionally stays anchored to well-formed https://, so malformed fragments remain in the text for other scans — that asymmetry looks deliberate and correct. New test (test_v2_rejects_malformed_https_reference_beside_a_valid_url) covers the motivating case. No ReDoS or exception-safety concerns; the character class excludes whitespace/brackets so matching stays linear, and URI::Error is already rescued.

Fix #2 (metric-name vocabulary) — logic gap in the safeguard it claims to keep

Left an inline comment on the specific issue: the new placeholder guard (skills/post-merge-audit/bin/closeout-evidence-replay:571) uses an exact full-string match (\A(?:metric|placeholder|tbd|todo)\z) instead of a tokenized check like the size_tokens check two lines below it. That means compound names such as metric_name=tbd_metric, metric_name=my_placeholder, or metric_name=todo_perf bypass the guard entirely — and since the runtime_tokens allowlist that used to be the other line of defense is now gone, nothing else catches them. It goes further: a metric name that's pure punctuation (e.g. metric_name=---) also validates, because text_tokens strips it to an empty array and every .any? check on empty arrays is vacuously false.

This matters because the PR description and CHANGELOG both assert the fail-closed "placeholder ... safeguards" are retained, but as written the safeguard only catches the four bare words, not the compound/gibberish forms an evidence-gaming attempt would actually use. I'd suggest tokenizing that check the same way size_tokens is checked, and consider explicitly rejecting metric names that tokenize to nothing.

I also left a suggestion on the test file: the new accept-path tests (checkout_ready, checkout_error_count) are good, but there's no reject-path test for the compound-placeholder-token case, which is likely why the gap wasn't caught.

Other notes

• Test rename (test_v2_measured_metric_requires_named_non_size_runtime_or_user_metric → ...requires_a_named_non_size_non_placeholder_metric) accurately reflects the new semantics, and the removed invalid-evidence cases (test_count, widgets_clicked, etc.) correctly track the intentionally loosened vocabulary.
• CHANGELOG entry is a single dense bullet consistent with the file's existing style.
• The PR description's "Codex Decision Log" reasoning for dropping the closed vocabulary (shared pack can't know a consumer's legitimate metric names) is sound in principle — my concern is specifically that the placeholder-rejection replacement for that removed allowlist is incomplete, not that loosening the vocabulary itself was wrong.

No security issues beyond the above (URL parsing already guards against SSRF-adjacent host tricks like localhost/127.0.0.1/private image host, unchanged by this PR). No performance concerns — regex changes stay linear and the script only runs over small evidence-marker text blocks.

[claude]

Review

Overview

This PR is a targeted follow-up to #262, fixing two P2 findings in closeout-evidence-replay:

1. A malformed https: reference (missing //) next to a valid URL was previously ignored rather than rejected, because http_urls only matched fully-formed https://... strings.
2. The measured_metric validator enforced a fixed, non-portable vocabulary of "runtime/user" tokens (cpu, latency, fps, etc.), rejecting legitimate consumer-defined metric names (e.g. checkout_error_count) that don't happen to contain one of those words.

Both fixes are narrow, well-scoped, and backed by new/updated tests (93 runs / 845 assertions per the PR description).

Code quality

• The rewritten valid_measured_metric? correctly inverts the approach from an allowlist to a denylist (placeholder/size/structural-count tokens), which matches the stated intent in the decision log: "validate portable structure and defer semantic truth to the consumer seam." This is a reasonable tradeoff for a shared/portable pack that can't know a consumer's domain vocabulary.
• structural_metric_units? is a clean, small helper — correctly requires all measurement units for a given token (e.g. chunk) to match that token's known unit set before rejecting, so a mixed/ambiguous metric name isn't falsely excluded (verified against the file_upload_latency test case).
• Left an inline comment on http_urls: the broadened regex catches malformed https: fragments for the pass/fail decision, but the sibling without_https_urls (used to strip URLs before token-based parsing elsewhere) wasn't updated to match, so the two helpers now disagree on what counts as a URL. It doesn't change any verdict today (the .all? check in http_url? already fails fast on the malformed entry), but it can add a spurious visual_evidence.local_reference entry to missing alongside the real visual_evidence.url one. Low severity, but worth tightening for consistency.

Test coverage

• Good coverage of the new behavior: malformed-https-beside-valid-url, uppercase HTTPS:// acceptance, placeholder tokens appearing anywhere in the metric name, consumer-defined duration/count metrics, and the new structural-count carve-out (chunk_count/file_count/module_count/test_count) with a companion "don't over-reject runtime names" case (file_upload_latency).
• Old tests that encoded the removed semantic-vocabulary requirement were correctly updated/removed rather than left stale.

Security / risk considerations

• This is an internal audit/validation script gating QA evidence in PR closeout, not a user-facing security boundary — the loosened metric-name validation is an intentional, low-risk tradeoff (documented in the decision log) rather than a vulnerability. The URL-validation change is a net-stricter fix (rejects more malformed input), which is the right direction for a "durable evidence" gate.

Performance

• No concerns — all changes are small regex/token operations on short strings, no algorithmic complexity changes.

Overall: solid, well-tested fix with clear rationale. One minor consistency nit flagged inline (regex helper drift between http_urls and without_https_urls); nothing blocking.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6cb049c47f

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfe89b7e40

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

[claude]

Review: Harden durable evidence replay edge cases

Overview

This PR modifies closeout-evidence-replay to (1) reject malformed https: references even when a valid URL appears alongside them, and (2) relax the measured-metric name/unit validation from a closed semantic vocabulary to a structural check that defers semantic truth to the consuming repo, while keeping placeholder/size/structural-count/byte-memory safeguards. Includes CHANGELOG entry and new/updated tests.

What looks good

• HTTPS_TOKEN broadening https://[^\s<>()]+ → https:[^\s<>()]* is a clean fix: http_urls/without_https_urls now capture malformed https: tokens (e.g. https:broken) so valid_https_url? correctly fails the all? check in http_url?, and the same stripped text avoids spurious "local reference" diagnostics for media-looking malformed tokens. Verified this logic traces through correctly for both new tests.
• structural_metric_units? uses exact token-array membership (via text_tokens) rather than substring matching, avoiding false positives like "filesystem_latency" incorrectly matching "file".
• The relaxed metric-name policy is explicitly documented in the PR's decision log as an intentional tradeoff (can't distinguish a legitimate consumer metric from junk without repo-specific knowledge), which is the right call for a portable/shared validator.
• Good test coverage added for the HTTPS-token and structural-count cases, including case-insensitivity and "does not falsely flag runtime-named metrics" checks.

Issue found (posted inline)

• The new placeholder guard (metric_name.match?(UNRESOLVED_PLACEHOLDER)) doesn't catch underscore-joined compound names (e.g. metric_name=unknown_duration) because \b doesn't fire against an adjacent _. The token-list fallback on the next line only checks placeholder/tbd/todo, not the other UNRESOLVED_PLACEHOLDER words (unknown, missing, unavailable, unmeasured, not_available, not_measured). See inline comment for a concrete bypass example and suggested fix. This mirrors a pre-existing gap in valid_repo_seam_source?, but it's more exploitable here since metric names are much more likely to be freeform/underscored.

Minor observations (non-blocking)

• The widened metric-name acceptance means fully generic/junk names (e.g. metric_name=score with a non-byte unit) are now silently accepted where they previously weren't — this is the intended design per the decision log, but worth double-checking that downstream consumers of this evidence understand the validator no longer enforces any runtime/semantic vocabulary, only structural shape.
• No performance or security concerns — this is a self-contained regex/string validator with no external I/O, and the new regex has no ReDoS risk (simple character-class repetition, no nested quantifiers).

Overall a well-scoped, well-tested fix for the two carried P2 findings; one gap in the new placeholder check worth addressing before/after merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 979d834cb3

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

[claude]

Review

Traced the diff carefully (couldn't execute the Ruby test suite in this sandbox — ruby invocations required approval I didn't have — so this is a static trace against the described behavior and the added tests).

HTTPS_TOKEN regex widening (https://[^\s<>()]+ → https:[^\s<>()]*)

• Correctly turns https:broken into a matched-but-invalid token, so http_urls(...).all? { valid_https_url?(...) } now fails when a malformed https: reference sits beside a valid URL — matches the stated fix and the new tests.
• Also fixes a secondary issue: previously https:broken.png wasn't stripped by without_https_urls, so LOCAL_MEDIA_EXTENSION could false-positive on the .png inside it and add a spurious visual_evidence.local_reference finding. The wider strip removes that false positive too (covered by test_v2_malformed_media_looking_https_reference_does_not_add_local_reference_diagnostic).
• No ReDoS concern — single character-class quantifier, no nested/overlapping quantifiers.
• Edge cases (bare https:, bare https://) still resolve correctly because parse_https_url already requires a non-empty uri.host, independent of how greedily the token is captured.

STRUCTURAL_METRIC_UNITS / relaxed valid_measured_metric?

• Replacing the closed runtime-token allowlist with a reject-list (placeholder/tbd/todo, size tokens, structural counts, non-memory byte units) is a deliberate, documented tradeoff (per the PR's own Codex Decision Log) to defer semantic truth to the consumer seam. I traced structural_metric_units? against mixed cases like file_upload_latency (token "file" present but unit "ms", not "file(s)") and confirmed the per-group token+unit pairing avoids false rejections — matches the new test test_v2_measured_metric_rejects_structural_counts_without_rejecting_runtime_name_tokens.
• This does mean arbitrary metric names (e.g. metric_name=xyz123) now pass structural validation as long as they're not placeholders/size/structural-count — that's the intended, disclosed relaxation, not a bug.

Known deferred gap (already disclosed, not introduced by this PR): valid_repo_seam_source? still accepts source=https:broken as a structurally "valid" non-URL source, since it only special-cases sources starting with https?://. The PR description explicitly calls this out as pre-existing and out of scope, so no action needed here — just confirming the scope boundary holds.

One minor test-coverage nitpick left as an inline comment on the new /\Ametric\z/i guard, which isn't exercised by any test in this PR.

Overall: scoped, well-traced against its own test suite, and the two behavior changes match the PR description. No functional bugs or security issues found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e7d456006

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

[claude]

Review summary

Reviewed the diff in skills/post-merge-audit/bin/closeout-evidence-replay (and its test file). Overall the change is well-scoped, matches the two carried P2 findings described in the PR body, and is backed by solid new test coverage (camelCase/snake_case unresolved-token variants, singular/plural size tokens, malformed-https:-beside-valid-URL cases).

Verified as correct:

• HTTPS_TOKEN (/(?<![A-Za-z0-9+.-])https:[^\s<>()]*/i) correctly causes http_url?/http_urls to flag a malformed https:broken token even when a valid https://... URL sits right next to it, since http_url? requires all matched tokens to parse as valid HTTPS URLs. This fixes the "malformed reference beside a valid one" bypass described in the PR.
• Removing the old fixed runtime_tokens allowlist and replacing it with blocklist checks (unresolved/placeholder tokens, size tokens, structural bounded counts, byte-without-memory) is an intentional, documented design change (deferring semantic truth to the consumer seam) — consistent with the Codex decision log in the PR description.
• text_tokens's new camelCase splitting is only used by valid_measured_metric? and valid_bundle_hygiene?, and only adds finer-grained tokens without breaking existing acronym-only names (e.g. LCP stays a single token).
• load SCRIPT addition in the test file is safe — the script's exit(...) if $PROGRAM_NAME == __FILE__ guard won't fire when loaded from the test runner, so no unintended CLI execution as a side effect of the new direct CloseoutEvidenceReplay.valid_measured_metric? unit test.

One correctness gap found (left as an inline comment on closeout-evidence-replay:599): structural_metric_count? checks tokens.include?("count") (singular only), while STRUCTURAL_COUNT_TOKENS includes plural nouns (chunks, files, modules, tests). A metric named test_counts or file_counts (plural "counts") tokenizes to e.g. ["test", "counts"], so the singular-only check fails to match and the bounded structural-count safeguard silently doesn't apply — that metric name would incorrectly pass as a valid measured_metric. The new tests only exercise plural-noun + singular-count combinations (chunks_count, files_count, etc.), not plural counts, so this slipped through.

No security concerns beyond the pre-existing, explicitly-deferred repo_seam source-shape gap already called out in the PR's own decision log (tracked as separate follow-up work). No performance concerns — all regexes are linear/bounded, no catastrophic-backtracking risk introduced.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eefe1ac260

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

[claude]

Review Summary

What this PR does: Follow-up to #262 fixing two carried P2 findings in skills/post-merge-audit/bin/closeout-evidence-replay:

1. Reject malformed https: references (e.g. https:broken, bare https:) even when a valid https://... URL appears elsewhere in the same evidence string.
2. Replace the curated runtime_tokens semantic allowlist for measured_metric names with structural checks (reject placeholders/unresolved tokens, size tokens, and structural *_count combos), so consumer-defined metric names/units (e.g. checkout_error_count) aren't rejected just for not matching a fixed vocabulary.

Also adds camelCase-aware tokenization (text_tokens) so notApplicable/todoLatency-style names are caught the same as their snake_case equivalents, and load SCRIPT in the test file to support direct in-process calls into the module.

Verification

I traced the regex/URI logic by hand (Ruby execution was sandboxed in this environment):

• HTTPS_TOKEN = /(?<![A-Za-z0-9+.-])https:[^\s<>()]*/i correctly widens matching from https:// (required // + 1+ char) to bare https: with zero or more following chars, so https:broken and https: are now captured by http_urls/without_https_urls and subsequently fail valid_https_url? (since URI.parse gives a nil host for those). This closes the original gap where a malformed token beside a valid URL was silently ignored. Confirmed against the new tests including the media-extension edge case (https:broken.png) not leaking into the local_reference diagnostic.
• CamelCase splitting in text_tokens correctly keeps merged words like rateUnknownness as a single token so it isn't confused with the banned unknown token — this is a deliberate, correctly-implemented distinction.
• structural_metric_count? / SIZE_METRIC_TOKENS correctly reuse the module constants (no duplicated literals) and behave as tested (e.g. checkout_error_count passes, test_count is rejected).

Findings (posted inline)

• Design consideration: dropping the runtime_tokens requirement means valid_measured_metric? now accepts any non-blocklisted metric_name (e.g. score, x, result) paired with non-byte units — there's no longer any check that the name is metric-shaped at all, only that it isn't one of a few banned words. This is called out as an intentional trade-off in the PR's decision log, but it's a meaningful loosening of an anti-gaming validator, so flagging for explicit reviewer sign-off.
• Minor: metric_name.match?(/\Ametric\z/i) only blocks the singular metric; the plural metrics slips through untouched by any other check.

Other observations

• Test coverage is thorough for the two stated fixes (malformed-URL-beside-valid-URL, camelCase/snake_case unresolved tokens, singular/plural size tokens, structural counts vs. runtime names).
• No test explicitly documents/locks in the "generic non-descriptive name now passes" behavior called out above — might be worth an explicit test if that's the desired contract, so a future change doesn't accidentally re-tighten or further loosen it without notice.
• CHANGELOG entry and QA evidence markers follow existing repo conventions.
• No security concerns beyond the validator-strictness point above — this is offline text/URL validation with no external I/O, URI::Error is still handled defensively, and no injection surface is introduced.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30fa999cd7

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28911da5be

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

[claude]

Review summary

What this PR does: hardens closeout-evidence-replay (the post-merge QA evidence marker validator) in two ways:

1. Malformed https: references (e.g. https:broken) are now caught even when a valid URL appears alongside them in the same field — previously the URL regex required https:// literally, so a bare https:broken token was invisible to http_urls/without_https_urls and slipped through as long as a real URL was also present.
2. The measured_metric performance-evidence check drops the old closed allowlist of "runtime" tokens (cls, cpu, duration, ... ) and instead validates structure: reject unresolved placeholders (missing, n/a, not_applicable, ...), reject bare/paired size-shape tokens (asset, bundle, byte, size, ...), reject bare structural counts (file_count, chunk_count, ...), but otherwise accept any consumer-defined metric name/unit. text_tokens now also splits camelCase so notApplicable/fileUploadLatency-style names tokenize correctly.

Code quality

• Clean extraction into small, single-purpose predicates (unresolved_metric_tokens?, structural_metric_count?, size_metric_shape?) consistent with the file's existing style.
• Good test coverage for the new behavior: malformed-URL-beside-valid-URL, the local-media-extension false-positive that malformed URLs used to trigger, camelCase/snake_case placeholder forms, structural-count vs. runtime-compound-name disambiguation (module_load_count vs file_count), and plural/singular size tokens.
• The CHANGELOG entry accurately describes the behavior change.
• Both carried P2 findings from Require durable visual evidence for UI changes #262 look correctly addressed by this change.

Potential issues (left as inline comments)

• unresolved_metric_tokens? rejects a metric name if any token anywhere matches UNRESOLVED_METRIC_TOKENS (missing, unavailable, unknown, todo, tbd, ...). Since the PR's goal is to accept consumer-defined metric names, a legitimate name that merely contains one of these words as domain vocabulary (e.g. a todo-app's todo_sync_latency) would be rejected. Consider scoping this to placeholder-only or placeholder+suffix names rather than "contains this token anywhere."
• Placeholder detection can be evaded by fusing words with no separator or case change — metric_name=notapplicable (all lowercase, no underscore) isn't caught by either unresolved_metric_tokens? or UNRESOLVED_PLACEHOLDER, while the underscore/camelCase forms are. Minor since it's a one-line dodge of the gate.

Other notes

• Loosening the metric-name check from an allowlist to "reject known-bad shapes, accept everything else" is a real behavior/risk-surface change (documented and consciously decided in the PR's Codex Decision Log), but reviewers should be aware it meaningfully widens what counts as valid measured-metric evidence going forward.
• No security or performance concerns: regex changes are linear (no ReDoS risk), and the HTTPS_TOKEN change intentionally broadens URL-token matching to include malformed https: prefixes, which is stripped consistently everywhere without_https_urls is used (correctly fixing the local-media false positive along the way).

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/post-merge-audit/bin/closeout-evidence-replay`:
- Line 36: Update the HTTPS_TOKEN pattern in closeout-evidence-replay to stop
matching at evidence-field delimiters such as semicolons, so extraction removes
only the HTTPS URL and preserves metric_name, baseline_value, and other required
measurements. Ensure supported source=https://... repository seams remain valid,
including the other HTTPS extraction occurrence.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c2127eea-33e4-4158-9c6c-7cd936f5d078

📥 Commits

Reviewing files that changed from the base of the PR and between 979d834 and 28911da.

📒 Files selected for processing (2)

• skills/post-merge-audit/bin/closeout-evidence-replay
• skills/post-merge-audit/bin/closeout-evidence-replay-test.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• skills/post-merge-audit/bin/closeout-evidence-replay-test.rb

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3fc5d3dac9

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

[claude]

Review summary

Reviewed the durable-evidence-replay hardening (closeout-evidence-replay, closeout-evidence-replay-test.rb, CHANGELOG.md). Overall the change is well-tested (103 replay assertions covering the new URL-boundary and metric-classification logic) and the two carried P2 findings from #262 (malformed https: beside a valid URL, and the closed runtime-metric vocabulary) are addressed. Traced the new HTTPS_TOKEN regex and the valid_measured_metric?/size_metric_shape?/structural_metric_count? helpers by hand against the test suite; logic is internally consistent with what's tested.

Three things worth a second look (posted as inline comments):

1. Metric-name gate is substantially loosened, not just "unblocked." The old closed vocabulary of runtime tokens is replaced by a denylist, so arbitrary names like metric_name=score or metric_name=x with a plausible non-byte unit now satisfy measured_metric evidence. The PR's decision log explains this is intentional (can't distinguish checkout_error_count from a made-up label without repo-specific knowledge), which is a reasonable position — but worth an explicit sign-off given this is a fraud-resistance gate for QA evidence, and the loosening is significant.
2. HTTPS_TOKEN's semicolon-boundary lookahead hardcodes exactly 4 field names (source, metric_name, baseline_value, candidate_value). Any future field added near a URL in this marker schema needs this list updated too, or URL boundaries will silently misparse — no test would catch a forgotten update.
3. size_metric_shape?'s terminal-size check only covers 2–3 token metric names. A 4+-token name like total_uncompressed_bundle_size bypasses both the size-shape and structural-count checks; it's usually caught downstream by the byorte-unit check, but a non-byte-unit size claim with 4+ tokens could slip through.

None of these are correctness bugs against the stated contract — they're mostly about how much the fraud-resistance guarantees of this specific gate have shifted, and a couple of maintenance-coupling risks. No security issues found in the traditional sense (this is a text-pattern validator over PR/marker text, not code that executes or fetches based on that input).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad14ebb1b2

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

[claude]

Review: Harden durable evidence replay edge cases

Overview
This is a tight, well-scoped follow-up to #262 that fixes the two carried P2 findings:

1. https:broken-style malformed HTTPS references next to a genuinely valid URL are now rejected instead of silently ignored, by replacing the strict https:// regex with a new HTTPS_TOKEN pattern that also matches bare/malformed https: prefixes and stops at either whitespace/brackets, end-of-string, or a recognized ;field_name= boundary.
2. The old metric_name validator enforced a closed vocabulary of runtime tokens (cls, cpu, fcp, etc.), which rejected legitimate consumer-defined metrics like checkout_error_count. It is replaced with a structural check: reject empty/placeholder/unresolved names, reject size-shaped and structural-count-shaped names, otherwise accept - consistent with the PR's stated philosophy of validating shape, not semantic truth.

Code quality

• The new helpers (unresolved_metric_tokens?, structural_metric_count?, size_metric_shape?) are small, single-purpose, and each has direct test coverage for the shapes they classify.
• text_tokens now does camelCase/acronym splitting (HTTPServer -> http server) in addition to the existing non-alnum split, a minimally invasive improvement that also benefits the existing valid_bundle_hygiene? caller.
• The HTTPS_TOKEN regex reuses PERFORMANCE_EVIDENCE_FIELD_NAMES to know where a URL boundary can legitimately be a semicolon-delimited field rather than URL content, backed by a dedicated test (test_v2_retains_semicolons_inside_https_url_content) for the semicolon-inside-URL-content case.
• Left two inline notes on subtleties worth a second look (not blockers): an asymmetry in how aggressively the two unresolved-metric-name checks apply based on token count, and a maintainability note that PERFORMANCE_EVIDENCE_FIELD_NAMES needs to stay in sync with the evidence schema for the URL-boundary lookahead to keep working correctly.

Potential issues / risks

• The metric-name validator is now intentionally very permissive - a single arbitrary token like x or result passes as a structurally valid metric name. This is a deliberate, documented trade-off (per the PR's Codex Decision Log) since the pack can't know consumer-specific semantics, but it's worth being aware this validator is a shape gate, not a garbage-data gate.
• No new external inputs, file I/O, or shell/process execution are introduced - this is pure text/regex validation, so no security concerns (no injection surface, no ReDoS risk from the new regex given its non-nested, single-char-class quantifier).

Test coverage
Very thorough - the diff adds roughly 20 new test cases covering malformed-HTTPS-beside-valid-URL, case-insensitive HTTPS://, camelCase/snake_case unresolved-token variants, structural count vs. runtime-name disambiguation (module_load_count vs total_file_count), size-shape terminal-position classification, and consumer-defined metric acceptance. This matches the PR's stated validation (103 runs / 1,205 assertions).

Verdict
No blocking issues found. The two inline notes are informational/food-for-thought rather than required changes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f91899e32

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

[claude]

Review summary

Reviewed the closeout-evidence-replay changes (durable URL parsing + measured-metric portability). I traced the new regex and helper logic by hand against the added test cases (couldn't execute the Ruby test suite in this environment, so this is static analysis, not a test run).

Overall assessment: the implementation is internally consistent and matches the extensive new test coverage well. I didn't find correctness bugs in the HTTPS_TOKEN extraction regex (it correctly stops URL capture at semicolon-delimited source=/metric_name=/baseline_value=/candidate_value= boundaries while preserving semicolons inside URL content, and correctly flags malformed https: fragments beside valid URLs) or in the camelCase tokenizer / qualifier look-through logic for size and structural counts.

Two things worth a second look before merge, left as inline comments:

1. Metric-name allowlist removal (valid_measured_metric?) — the old requirement that a metric name contain recognized runtime/perf vocabulary is gone. Generic/short names like x, score, result now pass. This is explicitly called out as intentional in the PR's decision log, but it's a meaningful loosening of a release-blocking QA gate worth a final sanity check.
2. Asymmetric terminal-noun carve-out in size_metric_shape? — size/byte nouns are rejected regardless of prefix, but asset/bundle nouns (also in SIZE_METRIC_TOKENS) aren't given the same terminal treatment, so page_assets/response_bundle-style names slip through uncaught. Looks like an oversight rather than intended behavior, and it's untested.

No security concerns beyond the above (no injection risk from the interpolated field-name regex since those are fixed constants; no ReDoS risk from the new lazy-quantifier regex — it has no nested/overlapping quantifiers that would cause exponential backtracking).