Skip to content

fix(ci): strengthen doc-object markers in prompt and add extraction fallback#479

Open
jdcmsd wants to merge 1 commit into
mainfrom
fix/robust-doc-object-extraction
Open

fix(ci): strengthen doc-object markers in prompt and add extraction fallback#479
jdcmsd wants to merge 1 commit into
mainfrom
fix/robust-doc-object-extraction

Conversation

@jdcmsd
Copy link
Copy Markdown
Member

@jdcmsd jdcmsd commented Jun 5, 2026

two fixes

  1. Prompt change — make it explicit that the HTML comment markers are CI plumbing, not optional formatting, and must be present even when the doc object is embedded in the rollback comment
  2. Extraction fallback — make the Python script resilient enough to find the doc object even if a future model run varies the format slightly

@jdcmsd jdcmsd requested a review from a team as a code owner June 5, 2026 14:30
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • This PR fixes #123 or Fixes: #123

  • This PR closes #123 or Closes: #123

  • This PR resolves #123 or Resolves: #123

  • Other supported keywords: fix, fixed, close, closed, resolve, resolved
    Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

@claude
Copy link
Copy Markdown

claude Bot commented Jun 5, 2026

Claude finished @jdcmsd's task in 1m 25s —— View job


Code Review: Robust doc-object extraction

  • Read PR diff (origin/main...HEAD)
  • Review prompt (STEP 7) changes
  • Review extraction-script (fallback) changes
  • Post findings

Reviewed .github/workflows/ai_claude-rollback-safety.yml. The two changes are sound in intent, but the extraction path has one real correctness bug and a couple of robustness gaps worth a second look.

🔴 Stale comment wins — extraction grabs the first match, not the latest

The pre-merge job runs on every synchronize and posts a new doc-object comment each push (none are deleted). By merge time a PR with N pushes has N `` comments. Both the primary regex (line 267) and the fallback (line 279) use re.search, which returns the first match — i.e. the oldest doc object, describing the diff from the first push, not the merged state. The git note then permanently attaches an outdated summary.

This is pre-existing, but the PR's stated goal is "resilient enough to find the doc object," so it's in scope. gh pr view ... --jq '.comments[].body' preserves chronological order, so the fix is to take the last occurrence:

# Primary
matches = re.findall(r'(.*?)', text, re.DOTALL)
block = matches[-1] if matches else None

…and likewise re.findall(...)[-1] for the fallback <details> search.

Fix this →

🟡 Code-fence stripping is only in the fallback path

Lines 289–291 strip ```yaml fences, but only when the outer markers are missing. If a future model emits the markers and wraps the body in a code fence (exactly the "format drift" this PR guards against), the primary path (lines 272–275) passes the fences straight into the git note. The fence-strip belongs in both branches — ideally factor draft extraction into one helper applied after either match.

🟡 Fallback is silently coupled to the summary literal

The fallback keys off [^<]*Doc Object Draft[^<]* (line 280), which must stay character-compatible with the <summary>📄 Doc Object Draft …</summary> text the prompt tells the model to emit (line 158). These live ~120 lines apart with no cross-reference. If anyone edits the summary wording in STEP 7, the fallback breaks with no test catching it. Worth a code comment on both sides noting the coupling.

Minor / non-blocking

  • STEP 9b interaction: when the change is "Safe to Rollback," STEP 9b posts no comment. The new "embed in the rollback safety comment" guidance (lines 165–167) therefore can't apply in that branch — the model must fall back to a standalone comment. The prompt is still correct (STEP 7 offers both), just worth confirming the model doesn't skip posting the doc object entirely when there's no rollback comment to embed in.
  • The fallback's <details>...</details> non-greedy match assumes the doc object body contains no nested <details>; fine today, fragile if doc-object schema ever nests one.

Verdict

Direction is right and the fallback is a reasonable safety net. The first-match-vs-latest bug is the one I'd fix before merge — it can silently attach an outdated doc object to the merge commit. The fence-strip asymmetry and summary-literal coupling are cheap hardening on top.

Note: I can review but can't push changes to files under .github/workflows/ (GitHub App workflow-write restriction), so the fixes above are suggestions for a maintainer to apply.
· branch fix/robust-doc-object-extraction

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant