Carry forward seam doctor follow-up fixes#4171
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe fence-detection regex in ChangesFence Regex and Delimiter Logic
Adoption Guide Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR carries forward two post-merge fixes from the
Confidence Score: 5/5Safe to merge; all changes are confined to agent workflow tooling and documentation with no effect on the React on Rails application itself. The regex change is a well-scoped one-liner with direct regression coverage via two new tests. The test class refactor is structurally clean — class boundaries verified, the new class correctly includes the test helpers, and no methods are orphaned. The doc addition is accurate and addresses a real gap in adoption guidance. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Line from skill Markdown] --> B{update_fence_state\nmatches 3+ backticks or tildes?}
B -- no match --> C[Not a fence delimiter]
B -- match --> D{Currently inside a fence?}
D -- yes --> E{delimiter equals fence_delimiter?}
E -- no --> F[Mismatched closer - stay inside fence]
E -- yes --> G[Close fence]
D -- no --> H[Open new fence, record delimiter]
H --> I{Language tag executable?}
I -- yes --> J[executable_fence = true\nScan lines for placeholders]
I -- no --> K[executable_fence = false\nPlaceholders allowed]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Line from skill Markdown] --> B{update_fence_state\nmatches 3+ backticks or tildes?}
B -- no match --> C[Not a fence delimiter]
B -- match --> D{Currently inside a fence?}
D -- yes --> E{delimiter equals fence_delimiter?}
E -- no --> F[Mismatched closer - stay inside fence]
E -- yes --> G[Close fence]
D -- no --> H[Open new fence, record delimiter]
H --> I{Language tag executable?}
I -- yes --> J[executable_fence = true\nScan lines for placeholders]
I -- no --> K[executable_fence = false\nPlaceholders allowed]
Reviews (1): Last reviewed commit: "Support long Markdown fences in seam doc..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba955940d8
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/bin/agent-workflow-seam-doctor-test.rb (1)
319-352: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for longer closing fences.
The test suite now covers:
- Mismatched delimiter types (``` vs ~~~)
- Shorter closing fences not closing longer opening fences
However, there's no test for the case where a longer closing fence should (or shouldn't) close a shorter opening fence. For example:
```bash <follow-up prefix>Per CommonMark, the ````` should close the fence, but the current implementation at line 214 of `agent-workflow-seam-doctor` requires an exact match. Adding this test case would help document whether the tool intentionally deviates from CommonMark or if this is an edge case to address. <details> <summary>📝 Suggested test case</summary> ```ruby def test_longer_closing_fence_closes_shorter_opening_fence with_repo do |root| write_agents(root) write_skill(root, <<~MARKDOWN) ```bash gh issue create --title "safe command" ````` <follow-up prefix> MARKDOWN out, status = run_doctor(root) # If longer fences should close per CommonMark: expect success # If exact match is intentional: expect failure and update comment assert status.success?, "longer closing fence should close shorter opening fence" end end ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/bin/agent-workflow-seam-doctor-test.rb around lines 319 - 352, Add missing test coverage for the edge case where a longer closing fence encounters a shorter opening fence. Create a new test method called test_longer_closing_fence_closes_shorter_opening_fence that writes a skill with a three-backtick opening fence for bash and a five-backtick closing fence, then runs the doctor and asserts the appropriate behavior (either success if longer fences should close per CommonMark, or failure if the implementation requires exact fence matching). This will document whether the tool intentionally deviates from CommonMark specification or if this is an edge case needing attention. ``` </details> <!-- cr-comment:v1:adab66c089c71f50dd8559af --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Nitpick comments: In @.agents/bin/agent-workflow-seam-doctor-test.rb: - Around line 319-352: Add missing test coverage for the edge case where a longer closing fence encounters a shorter opening fence. Create a new test method called test_longer_closing_fence_closes_shorter_opening_fence that writes a skill with a three-backtick opening fence for bash and a five-backtick closing fence, then runs the doctor and asserts the appropriate behavior (either success if longer fences should close per CommonMark, or failure if the implementation requires exact fence matching). This will document whether the tool intentionally deviates from CommonMark specification or if this is an edge case needing attention. ``` </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `7a38a02b-3dca-46e2-b375-11fefe296a7d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d92d92c1b8e105e7dc8e22c61680936e1e8c371a and ba955940d86886bc850083f562c3598e08ae21b3. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `.agents/bin/agent-workflow-seam-doctor` * `.agents/bin/agent-workflow-seam-doctor-test.rb` * `internal/contributor-info/agent-workflow-adoption.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Code review complete - see inline comment at line 209. Summary: The regex change correctly extends fence detection to 3+ backtick/tilde sequences per CommonMark spec. Test class reorganization is clean, and the two new tests cover the important cases. Docs update adds the missing nuance about workflow references. One spec gap: the closing-fence comparison uses exact string equality, so a longer closer (5 backticks closing a 4-backtick opener) is not recognized as a valid closer per CommonMark 4.5. Negligible in practice for skill files. Overall verdict: approve. |
justin808
force-pushed
the
jg-codex/4121-seam-doctor-followup
branch
from
222a282 to
86e331b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e331bb8b
ℹ️ 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".
Code ReviewSummary: Follow-up to #4121 — teaches the seam doctor CommonMark-compliant code fence parsing (3+ backtick/tilde openers, same-character closers at least as long as the opener) and clarifies shared-workflow adoption docs. Low risk, agent-tooling only. Implementation: The regex change to Tests: Splitting into Documentation: The added sentence in step 4 fills a real gap: installing only Verdict: Approve. The CommonMark fence fix is correct, test coverage of the new behaviour is solid, and the doc update addresses a concrete adoption pitfall. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e67784bbc
ℹ️ 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".
Code ReviewOverviewThis PR carries forward two post-merge P2 fixes that landed on the closed branch after #4121 was squash-merged. The changes teach Code Quality:
|
| Test | What it pins |
|---|---|
test_executable_placeholder_in_long_code_fence_fails |
4-backtick opener is now recognized |
test_shorter_closing_fence_does_not_close_long_executable_fence |
shorter closer stays inside |
test_shorter_closing_tilde_fence_does_not_close_long_tilde_fence |
same for tilde |
test_longer_closing_fence_closes_long_executable_fence |
longer closer is valid |
test_longer_closing_tilde_fence_closes_long_tilde_fence |
same for tilde |
test_closing_fence_with_info_string_stays_inside_executable_fence |
info string on closer is rejected |
One gap: there's no test for a backtick closer attempting to close a tilde-opener or vice versa when the lengths match (e.g., ~~~bash / ```). The test_mismatched_fence_delimiter_does_not_close_executable_fence covers tilde-close-of-backtick, but not backtick-close-of-tilde. The closing_fence_delimiter? logic handles it correctly via delimiter[0] == fence_delimiter[0], so this is a minor coverage gap, not a bug.
Documentation
The Step 4 rewrite is clear and actionable. The added sentence — "Do not run an installed-skill-only setup with only skills/: install workflows/ too, or keep repo-local workflow copies/launchers for skills that still reference .agents/workflows/..." — directly addresses the real failure mode (#4121 review threads linked in the PR). The prose is a bit dense but no worse than the surrounding guidance.
Validation
The PR's validation checklist is thorough: 36 tests / 93 assertions / 0 failures, RuboCop clean, git diff --check clean, shared-repo validate pass. Nothing is left for CI to catch here.
Summary
- Logic is correct and aligns with the CommonMark spec for closing fences
- The refactor is strictly better than the original — easier to reason about, no behavior regressions
- Test coverage is solid; the one missing inverse-mismatch test is not worth blocking on
- Docs change is accurate and useful
Approved with one suggestion: consider adding a test_backtick_closer_does_not_close_tilde_fence to round out the symmetric case, but this can be a follow-up if the author prefers to keep the PR scoped.
Code ReviewOverviewThis PR carries forward two post-merge review findings from #4121 and tightens the CommonMark closing-fence behavior in The three files changed are:
Fence Parser (
|
| Scenario | Test |
|---|---|
| 4-backtick opener detected | test_executable_placeholder_in_long_code_fence_fails |
| Shorter closer does not close longer fence (backtick) | test_shorter_closing_fence_does_not_close_long_executable_fence |
| Shorter closer does not close longer fence (tilde) | test_shorter_closing_tilde_fence_does_not_close_long_tilde_fence |
| Longer closer DOES close a long fence (backtick) | test_longer_closing_fence_closes_long_executable_fence |
| Longer closer DOES close a long fence (tilde) | test_longer_closing_tilde_fence_closes_long_tilde_fence |
| Info-string closer stays inside fence | test_closing_fence_with_info_string_stays_inside_executable_fence |
| CRLF line endings | test_crlf_closing_fence_closes_executable_fence |
| Spaced info string — non-executable | test_spaced_info_string_on_long_non_executable_fence_is_not_executable |
| Spaced info string — executable | test_spaced_info_string_on_long_executable_fence_is_executable |
One minor gap: there is no test for a tilde fence closed by a backtick closer (the reverse of the existing test_mismatched_fence_delimiter_does_not_close_executable_fence). The code handles it correctly (delimiter[0] == fence_delimiter[0] would be ` == ~, which is false), but the mirror case is absent. Not a blocker.
Documentation
The Step 4 update is clear and fixes a real footgun: users who followed the old advice and installed only skills/ (without workflows/) would break skills that reference .agents/workflows/.... The new wording makes the either/or explicit.
Summary
Approve. The logic is correct, the test coverage is thorough, and the doc improvement fills a genuine gap. The only suggestions are a style nit (local variable over parameter reassignment) and one missing mirror test case — neither is a blocker.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2f40337a-7ae3-4405-9ddf-8934c70ebafa) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7211630b-2efa-40aa-aac7-3ad98015a347) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code Review
OverviewThis is a focused, well-scoped follow-up to #4121 that improves CommonMark compliance in
|
| Area | Assessment |
|---|---|
| Fence parsing correctness | ✅ Correct per CommonMark spec |
| CRLF support | ✅ Properly handled via chomp |
| Test coverage | ✅ Comprehensive; minor gap on unclosed fences |
closing_fence_delimiter? safety |
|
| Caller/callee line chomping | |
| Documentation | ✅ Clear, actionable, consistent |
| Security / production risk | ✅ None; tooling-only changes |
Overall: solid work. The two
| [true, EXECUTABLE_FENCE_LANGS.include?(language), delimiter] | ||
| end | ||
|
|
||
| def closing_fence_delimiter?(delimiter, fence_delimiter) |
There was a problem hiding this comment.
This method assumes fence_delimiter is never nil (it calls fence_delimiter[0]). That invariant holds today because the only code path that sets in_fence = true simultaneously sets fence_delimiter to a non-nil regex capture in executable_placeholder_issues. Consider adding a guard or a comment expressing the precondition so a future refactor doesn't silently introduce a NoMethodError:
| def closing_fence_delimiter?(delimiter, fence_delimiter) | |
| def closing_fence_delimiter?(delimiter, fence_delimiter) | |
| # fence_delimiter is guaranteed non-nil when in_fence is true (set by the opening-fence branch). | |
| delimiter[0] == fence_delimiter[0] && delimiter.length >= fence_delimiter.length | |
| end |
| @@ -206,18 +206,31 @@ module AgentWorkflowSeamDoctor | |||
| end | |||
|
|
|||
| def update_fence_state(line, in_fence, fence_delimiter) | |||
There was a problem hiding this comment.
line.chomp is correctly applied here for CRLF support, but the caller (executable_placeholder_issues) continues to use the original un-chomped line object when it calls placeholders_from_line. This is harmless today (a trailing \r doesn't confuse SEAM_PLACEHOLDER scanning), but it means the two code paths operate on slightly different forms of the same line. If this method ever starts doing more string comparisons, this inconsistency could produce subtle bugs. One option is to chomp at the call site instead:
# In executable_placeholder_issues:
File.binread(path).force_encoding("UTF-8").scrub.each_line.with_index do |line, index|
line = line.chomp
...
end…ck-comparison-doc * origin/main: Bump version to 17.0.0.rc.6 docs: fix dead undici link and silence flaky GitHub links in lychee check (#4165) Update changelog for 17.0.0.rc.6 (#4174) Reduce PR security preflight false positives for CI metadata (#4170) [codex] Document targeted agent coordination reads (#4166) Carry forward seam doctor follow-up fixes (#4171) Unskip Rspack RSC e2e tests on react-on-rails-rsc 19.2 (rc.3) (#4173) Revert "docs: add accessibility best-practices guide (RoR + RSC) (#4086)" docs: rsc-css render-blocking links + end-of-head cascade trap (#4138) (#4143) Fix RSC-safe locale default messages output (#4146) Share agent workflows via installed skills + repo seam (#4121) Codify degraded agent-coord batch fallback (#4161) [codex] Document flagship demo coordination (#4164) Enrich deferred-render RSC errors with the bundle diagnostic (#3475) (#4100)
…tion-path * origin/main: docs(agents): make completing an authorized merge the expected close-out (#4156) docs: add accessibility best-practices guide (RoR + RSC) (#3927) (#4179) Bump version to 17.0.0.rc.6 docs: fix dead undici link and silence flaky GitHub links in lychee check (#4165) Update changelog for 17.0.0.rc.6 (#4174) Reduce PR security preflight false positives for CI metadata (#4170) [codex] Document targeted agent coordination reads (#4166) Carry forward seam doctor follow-up fixes (#4171) Unskip Rspack RSC e2e tests on react-on-rails-rsc 19.2 (rc.3) (#4173) Revert "docs: add accessibility best-practices guide (RoR + RSC) (#4086)" docs: rsc-css render-blocking links + end-of-head cascade trap (#4138) (#4143) Fix RSC-safe locale default messages output (#4146) Share agent workflows via installed skills + repo seam (#4121) Codify degraded agent-coord batch fallback (#4161)
…a-lane * origin/main: docs(agents): make completing an authorized merge the expected close-out (#4156) docs: add accessibility best-practices guide (RoR + RSC) (#3927) (#4179) Bump version to 17.0.0.rc.6 docs: fix dead undici link and silence flaky GitHub links in lychee check (#4165) Update changelog for 17.0.0.rc.6 (#4174) Reduce PR security preflight false positives for CI metadata (#4170) [codex] Document targeted agent coordination reads (#4166) Carry forward seam doctor follow-up fixes (#4171) Unskip Rspack RSC e2e tests on react-on-rails-rsc 19.2 (rc.3) (#4173) Revert "docs: add accessibility best-practices guide (RoR + RSC) (#4086)" docs: rsc-css render-blocking links + end-of-head cascade trap (#4138) (#4143) Fix RSC-safe locale default messages output (#4146) # Conflicts: # .agents/skills/plan-pr-batch/SKILL.md
1 participant
Summary
68db6f1agent-workflow-seam-doctorto scan 3+ backtick/tilde executable fences, reject too-short closers, accept same-character closers at least as long as the opener, reject info strings on closing fences, handle CRLF closers, and parse spaced info-string languagesworkflows/withskills/, or keep repo-local workflow copies/launchers for skills that reference.agents/workflows/...shakacode/agent-workflows@d38645bRationale
#4121 created and referenced the public shared workflow repo, but two late P2 review findings arrived after the merged head. The shared repo already contains the fixes; this PR keeps the React on Rails compatibility copy and adoption docs in sync with the published pack. Current-head review on this follow-up also tightened the CommonMark closing-fence behavior, now fixed here and in the shared repo.
The shared pack now also includes host-aware install/status/upgrade helpers inspired by the gstack upgrade flow: explicit status tokens, Codex/Claude host targets, network disclosure, rollback if a consumer seam validation fails, and preservation of unrelated user-installed agent files.
Original #4121 review threads:
Changelog
Not user-visible; agent workflow docs/tooling only.
Validation
ruby .agents/bin/agent-workflow-seam-doctor-test.rb-> 39 runs, 98 assertions, 0 failures.agents/bin/agent-workflow-seam-doctor --shared /Users/justin/codex/agent-workflows-> PASS(cd /Users/justin/codex/agent-workflows && bin/validate)-> PASS atd38645bgit diff --checkin bothreact_on_railsandagent-workflows-> cleanbin/ci-local --changed-> docs-only, Docs Sidebar Check passed, no further CI neededNote
Low Risk
Changes are limited to agent workflow tooling and internal documentation; no application runtime, auth, or user-facing product code paths are modified.
Overview
Syncs the repo’s agent-workflow compatibility copy with post-#4121 fixes from
shakacode/agent-workflows, focused on seam validation and adoption docs.agent-workflow-seam-doctornow parses Markdown code fences closer to CommonMark: 3+ backtick/tilde openers, closers must match character and be at least as long as the opener (shorter closers stay inside the fence), closing lines with info strings do not close a fence, CRLF closers work viachomp, and spaced info strings (e.g.``` bash) still mark executable fences. A large new test suite covers long fences, mismatched delimiters, and non-executable fenced content.Docs/skills broaden “Codex batch” wording to agent / Codex / Claude batches in
AGENTS.md,pr-batch, and contributor guides. Adoption and design docs addagent-workflows-statusandupgrade-agent-workflowsfor Codex and Claude hosts, stress installingworkflows/alongsideskills/, and link the shared pack’s installation/upgrade guide.Reviewed by Cursor Bugbot for commit 924b4eb. Bugbot is set up for automated code reviews on this repo. Configure here.