Skip to content

fix(factory): a section heading matches regardless of case, so a lower-case contract is still a contract - #1608

Open
fairchild wants to merge 2 commits into
mainfrom
workspace/claude-1598-heading-case
Open

fix(factory): a section heading matches regardless of case, so a lower-case contract is still a contract#1608
fairchild wants to merge 2 commits into
mainfrom
workspace/claude-1598-heading-case

Conversation

@fairchild

Copy link
Copy Markdown
Owner

Summary

  • An issue written ## Requested evidence (ws send silently truncates long text from the front, and reports full delivery #1450) extracted no contract, while ## Requested Evidence (factory-implement: an admission skip on label state leaves no trace, so a released issue sits ready forever #1558) extracted four items. Section headings now match regardless of case in the contributor helpers (markdown_section, has_markdown_section, strip_markdown_section, insert_markdown_section) and in sync-execution-state.py's copy of markdown_section. Everything else about the match stays as it was: start of line, ## , and the end at the next ## or ---.
  • insert_markdown_section's placement match takes the same flag. Without it, a lower-case before-heading passes the presence check, matches nothing, and the new section is dropped without a word. The replacement is now a function that keeps the heading as written. That also stops a backslash in the section from being read as a regex escape: a \d in a validation note raised re.PatternError on main.
  • The issue's item 2 (say so when an issue has no contract) is covered by this change and needs no new comment path. Admission's no_evidence_contract decline and April's accounting both read the contract through extract_requested_evidence. A lower-case heading is now a contract in both places, and an issue with no section at all still gets the decline comment admission already posts.

Mergeability

  • Surface: agent-runtime / factory — the section matchers in .agents/skills/cofounder-contributor/scripts/_helpers.py and the markdown_section copy in sync-execution-state.py
  • User-facing behavior changed: a ## Requested evidence heading in any case is a contract at admission and at review; ## evidence status in a PR body counts as the status section; a hand-written section in another case is replaced, not duplicated, when the factory re-renders it; ## Blocked by blocks
  • Non-happy paths considered: a lower-case before-heading keeps the inserted section (mutation-checked); two sections differing only in case: the first is read, which is the rule duplicate same-case sections already follow; output for canonical-case bodies is byte-identical to main
  • Release/ops preconditions: None
  • Residual risk or follow-up: three matchers outside this change are still case-sensitive, and each fails visibly. pr-readiness.py's extract_section means an agent-written ## mergeability is now kept, where it was previously supplemented with a seeded block, so readiness reports the section missing. evidence.py's literal ## Evidence Status in _insert_evidence_metadata and in the legacy reconciler means a hand-written lower-case status section's pending-ci lines stay pending, and its metadata lands at the end of the body, where the reader still finds it. In a PR body, readiness's evidence_status_heading_failure already rejects any variant of that heading, so the accounting now reads a lower-case status section but the gate still asks for the exact one. That strictness is unchanged here. A follow-up could route those three through the shared rule.

Validation

  • swift test: n/a, no Swift touched
  • Other checks run: the brief's gates, all bare, on head 42ab12ee:
    • uv run --script scripts/tests/test_factory_evidence_kinds.py -> Ran 189 tests in 0.092s / OK
    • uv run --script scripts/tests/test_factory_evidence_review.py -> Ran 17 tests in 0.003s / OK
    • uv run --script scripts/tests/test_factory_evidence_verify.py -> Ran 31 tests in 0.005s / OK
    • uv run --script scripts/tests/test_run_contributor.py -> Ran 113 tests in 1.295s / OK
    • uv run .agents/scripts/test_parse_frontmatter.py -> Ran 23 tests in 0.000s / OK
    • uv run .agents/scripts/test_run_planner.py -> Ran 113 tests in 0.079s / OK
    • uv run .agents/skills/gh-discuss/scripts/test_gh_discuss.py -> === Unit Tests: 14 passed, 0 failed ===
    • python3 scripts/check-subprocess-timeouts.py -> OK: no un-timed ProcessRunner.run calls in 318 files
  • Red first: SectionHeadingCaseTests against main's helpers printed FAILED (failures=7) across its first five tests, and the backslash case raised re.PatternError: bad escape \d. Mutation: restoring case-sensitive matching throughout turns all five red. Leaving only the placement match case-sensitive turns the insert-before test red, with the new section missing from the output. Leaving only sync-execution-state.py case-sensitive turns the Blocked by test red. So does enabling the flag for the two evidence headings alone.
  • Live specimen: ws send silently truncates long text from the front, and reports full delivery #1450's body extracts 0 items on main and 3 on this branch; factory-implement: an admission skip on label state leaves no trace, so a released issue sits ready forever #1558's extracts 4 on both.

Review loop

A directed codex pass (gpt-5.6-sol, xhigh) on e4928ced found nothing blocking.

  • Minor, fixed in 42ab12ee: the tests proved case tolerance for the two evidence headings, not the helper's rule. Enabling the flag for those two alone kept every test green while github_state.py's ## Blocked By reader regressed. The Blocked by test now asserts the shared helper's reader as well as the sync copy, and that mutation goes red.
  • Checked and sound: every edited line's revert turns its test red; output for canonical-case bodies is identical to main across 95 inputs (CRLF, trailing heading whitespace, duplicate canonical headings, empty bodies); a lower-case section shadowing a canonical one grants a body author nothing they couldn't already do with two canonical headings; the three matchers outside the change fail visibly, not silently.

Performance

  • Not a performance-sensitive change

Evidence

  • Tests named below: the command and the line it printed

Evidence Status

  • [complete] A test in scripts/tests/test_factory_evidence_kinds.py (or the helpers' tests) asserting ## Requested evidence and ## Requested Evidence extract the same items, red before the change -- uv run --script scripts/tests/test_factory_evidence_kinds.py -> Ran 189 tests in 0.092s / OK; test_a_lower_case_requested_evidence_heading_is_the_same_contract asserts Requested evidence, requested evidence and REQUESTED EVIDENCE extract the two items ## Requested Evidence does, and it was red against main's helpers
  • [complete] Every scripts/tests/*.py passes under uv run --script -- uv run --script scripts/tests/*.py, looped over find scripts/tests -type f -name '*.py' the way ci-agents.yml runs it, on head 42ab12ee -> 51/51 files passed

Blockers

  • None

Closes #1598

🤖 Generated with Claude Code

https://claude.ai/code/session_01QJmghYakhPSTXHJzxSXezL

Orchestrator note (workspaces)

fairchild and others added 2 commits September 11, 2026 04:16
…r-case contract is still a contract

`## Requested evidence` (#1450) extracted no items while `## Requested
Evidence` (#1558) extracted four, so one issue got no contract and nobody
was told. The section helpers and sync-execution-state.py's copy now
match the heading case-insensitively; start of line, `## `, and the end
at the next `## ` or `---` are unchanged.

insert_markdown_section's placement match takes the same flag, or a
lower-case before-heading would pass the presence check and then match
nothing, dropping the new section. Its replacement is now a function
that keeps the heading as written, which also stops a backslash in the
section from being read as a regex escape.

Closes #1598

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QJmghYakhPSTXHJzxSXezL
In response to codex (gpt-5.6-sol, xhigh), minor: the tests proved case
tolerance for the two evidence headings, not the helper's rule. A
mutation enabling IGNORECASE only for "Requested Evidence" and
"Evidence Status" kept every test green while github_state.py's
`## Blocked By` reader regressed. The sync-copy test now also asserts
the shared helper's extract_blocked_by, and that mutation goes red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QJmghYakhPSTXHJzxSXezL
@fairchild fairchild added the author:claude-code PRs authored by the claude-code agent (interactive Claude Code sessions) label Sep 11, 2026
@vercel

vercel Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
workspaces Ready Ready Preview Sep 11, 2026 11:36am UTC

Request Review

@fairchild

Copy link
Copy Markdown
Owner Author
  • 2026-09-11T11:38:19Z progress | gate on head 42ab12e (origin/main 5b6bb9d)
  1. Bare in the worktree: test_factory_evidence_kinds.py Ran 189, OK; test_factory_evidence_review.py Ran 17, OK; test_factory_evidence_verify.py Ran 31, OK; test_run_contributor.py Ran 113, OK; test_pr_readiness.py Ran 52, OK; .agents/scripts frontmatter and planner tests 23 + 113 OK; full scripts/tests loop 51 files, 0 failed; check-subprocess-timeouts.py OK.
  2. Independent mutation check: origin/main's _helpers.py and sync-execution-state.py under the new tests → FAILED (failures=7, errors=1), worktree restored clean.
  3. Diff read: (?i) on markdown_section, has_markdown_section, strip_markdown_section, on insert_markdown_section's before-heading match (which now keeps the existing heading's text through a capture instead of rewriting it), and on the sync script's own copy; nothing else about the match changed. The issue's second item falls out: a lower-case contract is now a contract, so admission's no_evidence_contract decline and April's accounting both see it.
  4. Evidence: A Requested Evidence heading in the wrong case is silently no contract at all #1598's two requested items read [complete] by the factory's parser; readiness passed on the body. CI test green on this head.
  5. MERGEABLE with headRefOid equal to the worktree HEAD at flip time.

Flipping ready. Merge is the portfolio steward's under B4, or Michael's click if its harness refuses again.

Orchestrator note (workspaces)

@fairchild
fairchild marked this pull request as ready for review September 11, 2026 11:38

@april-clearwater april-clearwater Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

April Clearwater, Application Lead

✅ Approve

A one-character regex fix ((?ms)(?msi)) that closes a real silent-failure gap, plus a bug fix to insert_markdown_section's replacement string that would have raised on a backslash in a validation note.

Evidence: ✅ satisfied — issue #1598 asked for exactly two items (a red-before-fix case-tolerance test, and the full scripts/tests/*.py suite green), and the PR's ## Evidence Status marks both [complete] with the command and the printed line for each.

Traced the fix through every call site that reads `## Requested Evidence` / `## Blocked By`, all consistent

What this change shape needed: named-test evidence, not a screenshot — this is a pure Python regex/logic fix in agent-runtime tooling, nothing a person looks at on screen. That's what's provided, with the exact command and the line it printed for both requested items, and the "red first" note confirms the new test actually failed against main before the fix.

Traced the fix, not just read it:

  • markdown_section, has_markdown_section, strip_markdown_section in _helpers.py all gain the i flag — straightforward.
  • insert_markdown_section's placement substitution changes from re.sub(pattern, f"{section}\n\n## {before_heading}", ...) to a capturing group plus a lambda replacement. This is a genuine second bug fix beyond the stated case issue: re.sub reads backslashes in a literal replacement string as its own escapes, so a \d in an inserted validation note would raise re.PatternError — the new test_the_inserted_section_is_written_as_text catches exactly this.
  • sync-execution-state.py carries its own copy of markdown_section (not an import), and it gets the identical one-line fix. Confirmed by reading the file directly.
  • github_state.py's extract_blocked_by and evidence.py's extract_requested_evidence both import markdown_section from the now-patched _helpers.py rather than defining their own — so the fix reaches both the admission path (no_evidence_contract decline) and April's review-time accounting without further changes, as the PR body claims. Verified by reading both files' imports directly.
  • The new SectionHeadingCaseTests class exercises all four call sites: requested-evidence extraction, evidence-status detection, section re-rendering (replace not duplicate), placement-insert with a lower-case before-heading, the backslash-in-replacement case, and both extract_blocked_by readers.

Residual risk, disclosed not hidden: the PR body names three matchers left case-sensitive outside this change (pr-readiness.py's extract_section, evidence.py's literal ## Evidence Status in _insert_evidence_metadata, and the readiness gate's heading check) and explains each fails visibly rather than silently. That's a reasonable scope boundary for a fix this size, and a fair follow-up to file separately rather than fold in here.

No UI/UX surface in this change — outside my usual lane, but the logic traces cleanly and the tests match the claims.

@fairchild

Copy link
Copy Markdown
Owner Author

Codex pass — 2026-09-11, head 42ab12e

One directed pass (gpt-5.6-sol, xhigh) over the diff against #1598's ask, the correctness of the match itself, whether the tests catch a regression in either copy, and anything outside the issue's fence. Three findings, all minor; none blocks the merge.

  1. scripts/pr-readiness.py:118, .agents/skills/cofounder-contributor/scripts/evidence.py:691, evidence.py:2589minor. Case tolerance stays partial. These three readers keep rf"(?ms)^## ...", r"(?m)^## Evidence Status\s*$", and line.strip() == "## Evidence Status". The PR body describes the resulting mixed readings accurately:

    • A lower-case ## Mergeability is now kept by seed_mergeability_section, and then readiness reports the section missing.
    • Accounting reads a lower-case ## evidence status, and readiness sees it as a variant and reports an ambiguous heading.
    • The legacy reconciler leaves a lower-case section's pending-ci lines unchanged, accounting still reads them as pending, and readiness rejects the variant.

    Each path fails out loud, so none of them reopens the silent-contract hole A Requested Evidence heading in the wrong case is silently no contract at all #1598 is about. Inconsistent and worth a follow-up, not blocking here.

  2. .agents/skills/cofounder-contributor/scripts/sync-execution-state.py:280minor. The local markdown_section is character-for-character identical to _helpers.py:148, and nothing appears to force the duplication: both files sit in the same scripts directory, _helpers.py imports only the standard library, and six sibling scripts already do from _helpers import ... under uv run --script. Avoidable historical drift. The new two-reader test does now catch the two copies diverging.

  3. .agents/skills/cofounder-contributor/scripts/_helpers.py:177minor. The callback replacement lambda match: f"{section}\n\n{match.group(1)}" fixes a bug outside A Requested Evidence heading in the wrong case is silently no contract at all #1598's fence. The old literal replacement really does raise re.PatternError: bad escape \d on a literal \d in the section. The callback keeps the content verbatim, keeps the matched heading's case, and still replaces only the first before-heading. Correct and tested — it is simply a second bug fix riding along.

Match behavior checked. re.IGNORECASE also equates the dotted and dotless i and the Kelvin sign with their ASCII letters, where casefold() differs on the Turkish pair, and it does not expand ß to ss; the heading names are fixed ASCII, so none of that bites. The flag changes case and nothing else: a colon after the heading and a double space after ## still do not match. Trailing heading whitespace and CRLF are still accepted by has_markdown_section but not by the content readers or the stripper, which is a pre-existing asymmetry this change neither creates nor widens. With two sections differing only in case, markdown_section reads the first, has_markdown_section reports present, strip_markdown_section removes both, and a re-render leaves one canonical section; placement takes the first matching before-heading. Shadowing was already available to a body author with two canonical headings, and readiness rejects Evidence Status variants either way.

Mutation coverage is complete. Each edited line has a test that goes red when it is reverted: _helpers.py:149 by the requested-evidence, evidence-status accounting, and shared Blocked By tests; :157 by the presence and lower-case placement tests; :161 by the re-rendering test; :176 by the lower-case placement test (on main the section lands after the lower-case before-heading, so the ordering assertion fails); :177 by the literal-backslash test; sync-execution-state.py:281 by the second assertion in the two-reader Blocked By test. Restoring origin/main's logic under the new tests reproduced FAILED (failures=7, errors=1) exactly, matching the gate comment. Branch tests pass: 189 evidence-kind, 52 readiness, 113 contributor. Live parsing confirms #1450 moves from 0 items to 3 while #1558 stays at 4.

Not verified

Verdict: FINDINGS

Steward note (steward v9)

@april-clearwater

Copy link
Copy Markdown
Contributor

All three findings match the PR body's own residual-risk and drift notes, and none blocks — confirming that stance rather than adding a new one.

On finding 2, the duplicated markdown_section: sync-execution-state.py doesn't import from _helpers.py, unlike its six siblings, so this change kept the copy in sync by hand rather than fixing the drift. A follow-up should switch it to from _helpers import markdown_section and delete the local copy, then confirm the two-reader Blocked by test still holds with a single shared function instead of two identical ones.

On finding 1, the three case-sensitive readers (pr-readiness.py:118, evidence.py:691, evidence.py:2589): agreed these fail visibly rather than silently, so they're out of scope for #1598. Worth its own issue to route them through the same (?i) rule, since the PR body already documents the three concrete failure modes (## Mergeability reported missing, ## evidence status reported ambiguous, a lower-case status section's pending-ci lines stuck).

No action needed on this PR from these findings — recording the sync-execution-state.py import cleanup and the three-reader follow-up as separate issues is a task for whoever picks up next, not a blocker here.

@fairchild

Copy link
Copy Markdown
Owner Author
  • 2026-09-11T11:56:35Z progress | codex minors on head 42ab12e, adjudicated without a push

Codex triage — head 42ab12e

Adjudicated: taken 1, declined 1, filed #1609 — head 42ab12e

  • filed — (1) case tolerance stays partial at scripts/pr-readiness.py:118, evidence.py:691, and evidence.py:2589; each fails loud and the body discloses all three. pr-readiness.py's extract_section still matches section headings case-sensitively #1609 already covers the readiness gate's matcher and now names the two evidence.py sites too, so one PR closes all three under one rule.
  • declined — (2) sync-execution-state.py:280's markdown_section is now character-identical to the helper and could import it. That consolidation is a refactor outside A Requested Evidence heading in the wrong case is silently no contract at all #1598's fence, and the new two-reader test fails if the copies diverge, which is the risk the finding names.
  • taken as disclosed — (3) the capture-preserving replacement in insert_markdown_section (_helpers.py:177) is a second fix the case tolerance exposed: without it a lower-case before-heading passed the presence check, matched nothing, and the new section was silently dropped. It is in the diff, tested, and named in the body.

Head stays 42ab12e.

Orchestrator note (workspaces)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author:claude-code PRs authored by the claude-code agent (interactive Claude Code sessions) mergeable Agent-approved, ready for owner merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A Requested Evidence heading in the wrong case is silently no contract at all

1 participant