Skip to content

chore: add agent workflow seam#778

Merged
justin808 merged 6 commits into
masterfrom
jg-codex/agent-workflows-seam-clean
Jul 11, 2026
Merged

chore: add agent workflow seam#778
justin808 merged 6 commits into
masterfrom
jg-codex/agent-workflows-seam-clean

Conversation

@justin808

@justin808 justin808 commented Jul 11, 2026

Copy link
Copy Markdown
Member

Summary

Shared ShakaCode workflows can now resolve this repository's validation and test commands, policy, trusted-actor defaults, and master base branch without copying workflow logic into the repo.

Validation

  • agent-workflow-seam-doctor --shared "$HOME/src/agent-workflows" — passed
  • .agents/bin/validate — passed (57 files inspected; no RuboCop offenses)
  • .agents/bin/test — attempted; 47 specs fail because the checkout lacks ssr-generated/server-bundle.js, a generated SSR artifact required by the existing system tests.

Post-Deploy Monitoring & Validation

No additional operational monitoring required: this change only adds repository-local agent workflow metadata and command wrappers.


Compound Engineering
GPT-5

Summary by CodeRabbit

  • Documentation

    • Added an “Agent Workflow Configuration” section to explain how shared skills map to standardized repository scripts.
    • Replaced the Agent Workflow scripts README with a standardized “Agent Workflow Scripts” reference.
  • Chores

    • Added agent workflow configuration metadata and trusted GitHub actor allowlists.
    • Added/updated repository entry-point wrappers (setup, validate, test, lint, build) to run in strict mode, execute from the repo root, forward arguments, and delegate through the conductor entrypoint; the test wrapper also manages the renderer process lifecycle and readiness.

@github-actions

Copy link
Copy Markdown

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:
They require the repository to have cpflow review apps configured, including the CPLN_TOKEN_STAGING secret.

+review-app-deploy

Deploy your PR branch for testing.

+review-app-delete

Remove the review app when done.

+review-app-help

Show detailed instructions, environment setup, and configuration options.

Comment +review-app-help for full setup details.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 11 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

Run ID: c9f1a615-df1d-43ed-8867-d06d6b1b1498

📥 Commits

Reviewing files that changed from the base of the PR and between 7405673 and 4ee4c07.

📒 Files selected for processing (1)
  • .agents/bin/test

Walkthrough

Added repository-level agent workflow configuration, standardized command entrypoints for setup, build, lint, test, and validation, trusted GitHub actor allowlists, renderer lifecycle handling, and documentation describing command and policy resolution.

Changes

Agent workflow setup

Layer / File(s) Summary
Workflow policy and configuration documentation
.agents/agent-workflow.yml, .agents/trusted-github-actors.yml, .agents/bin/README.md, AGENTS.md
Defines workflow metadata, trusted actor allowlists, standardized command mappings, and configuration resolution.
Standardized command entrypoints
.agents/bin/setup, .agents/bin/validate, .agents/bin/build, .agents/bin/lint
Adds strict Bash wrappers that resolve the repository root and delegate setup, RuboCop, build, and ESLint commands with forwarded arguments.
Renderer-backed test orchestration
.agents/bin/test
Checks or starts the renderer on port 3800, waits for readiness, cleans up spawned processes, and runs the CI pipeline.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • shakacode/react_on_rails#4228 — Concerns validation of the .agents/ workflow scripts, configuration, and AGENTS.md introduced by this change.

Sequence Diagram(s)

sequenceDiagram
  participant TestScript
  participant Renderer
  participant CIPipeline
  TestScript->>Renderer: Check port 3800
  TestScript->>Renderer: Start renderer when unavailable
  Renderer-->>TestScript: Port becomes ready
  TestScript->>CIPipeline: Run bin/ci
  TestScript->>Renderer: Stop spawned renderer on exit
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding an agent workflow seam for repo-local workflow metadata and wrappers.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/agent-workflows-seam-clean

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

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

Comment thread .agents/bin/test Outdated
Comment thread .agents/bin/README.md
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Summary

Small, additive PR that adds a repo-side "agent workflow seam" (.agents/) so shared ShakaCode workflow tooling can discover this repo's validate/test commands, trust policy, and base branch — no existing runtime code is touched aside from an appended section in AGENTS.md.

Overview

  • .agents/agent-workflow.yml — policy metadata (base branch, various n/a flags for unused shared-workflow features).
  • .agents/bin/{validate,test} — thin wrappers: validatebin/conductor-exec bundle exec rubocop, testbin/conductor-exec bundle exec rspec.
  • .agents/bin/README.md — documents the standard entry points and marks setup, lint, build, docs, ci-detect as n/a in this repo.
  • .agents/trusted-github-actors.yml — all trust lists empty (safe default).
  • AGENTS.md — short pointer section to the new seam.

Code quality

  • The bash wrappers use a safe, idiomatic pattern (set -euo pipefail, CDPATH= reset before cd, proper quoting) and correctly delegate to the existing bin/conductor-exec wrapper documented in AGENTS.md/CLAUDE.md.
  • Scripts are marked executable (mode 100755) as expected.
  • Everything here is metadata/config — no production code paths are affected, so blast radius is very low.

Issues found (posted inline)

  1. .agents/bin/test fails out of the box — the repo's specs depend on a generated SSR bundle (ssr-generated/server-bundle.js, per config/initializers/react_on_rails.rb) that's produced by a build step (e.g. yarn build:test), not by bundle exec rspec alone. The PR's own validation notes confirm 47 specs fail for this reason in a fresh checkout. Since setup/build are marked n/a, any shared workflow invoking .agents/bin/test will hit the same failures unless it happens to build first out-of-band.
  2. lint/test scoped to Ruby only — this is a full-stack repo with real JS-side lint (.eslintrc, yarn lint) and Jest coverage (existing *.spec.jsx files, yarn test:client), but the seam marks both n/a/Ruby-only. Any shared-workflow gating built on this seam will silently skip all JS linting and JS tests.

Security

  • trusted-github-actors.yml defaults to empty trust lists, which is the safe/conservative default — no concerns.
  • No secrets, credentials, or externally-reachable behavior introduced.

Test coverage

Overall this is low-risk, well-structured plumbing; the main actionable feedback is closing the gap between what the seam claims to validate/test and what actually gets exercised (Ruby-only, and only functional after a build step the seam doesn't provide).

@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds the repository seam used by shared agent workflows. The main changes are:

  • New workflow metadata under .agents/agent-workflow.yml.
  • New trusted GitHub actor allowlist file with empty defaults.
  • New .agents/bin/test and .agents/bin/validate command wrappers.
  • Documentation for the shared workflow command and policy locations.

Confidence Score: 4/5

The shared test entrypoint needs a fix before merging.

  • The validation wrapper is straightforward.
  • The new policy files do not create a concrete local security issue.
  • The test wrapper exposes a command that needs a generated SSR artifact, while setup and build seams are marked unavailable.

.agents/bin/test, .agents/bin/README.md

Important Files Changed

Filename Overview
.agents/agent-workflow.yml Adds base branch and workflow policy metadata for shared agent workflows.
.agents/bin/README.md Documents the available command wrappers and marks setup, build, lint, docs, and CI detection as unavailable.
.agents/bin/test Adds the shared test command wrapper, but it points at a test command that needs a generated SSR artifact without exposing setup or build.
.agents/bin/validate Adds the shared validation command wrapper for RuboCop.
.agents/trusted-github-actors.yml Adds empty trusted actor lists for users, bots, metadata bots, and teams.
AGENTS.md Documents where portable shared skills resolve commands and policy.

Reviews (1): Last reviewed commit: "chore: add agent workflow seam" | Re-trigger Greptile

Comment thread .agents/bin/test Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd56081da8

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

Comment thread .agents/bin/test Outdated
Comment thread .agents/bin/build Outdated
Comment thread .agents/bin/test Outdated
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review: chore: add agent workflow seam

Overview

Adds a small, self-contained "agent workflow seam": .agents/agent-workflow.yml (policy, mostly n/a for this repo), .agents/trusted-github-actors.yml (empty trust lists — safe default), five .agents/bin/* wrapper scripts (setup, validate, test, lint, build), a README documenting them, and an AGENTS.md section pointing at all of it. Pure scaffolding/config for shared ShakaCode tooling — no application code touched.

Code quality

  • The wrapper scripts are simple, consistent boilerplate (set -euo pipefail, cd to repo root, exec through bin/conductor-exec), which matches the project's existing conductor-compatibility convention documented in AGENTS.md/CLAUDE.md.
  • Verified each wrapped command exists and matches project conventions (bin/setup --skip-server, bin/rubocop, bin/ci, yarn lint:eslint, yarn res:build, yarn build:test all exist and behave as documented in .agents/bin/README.md).
  • Two inline issues flagged on the scripts themselves (see inline comments):
    • .agents/bin/build duplicates work already done inside yarn build:test (redundant generate_packs + res:build calls), and does so with a slightly different env (NODE_ENV not set) than the pass inside build:test — wasteful and a latent inconsistency.
    • .agents/bin/test doesn't forward "$@" like its sibling scripts do, which is an inconsistency worth fixing for a script explicitly meant to be a stable, generic seam for shared automation.

Security

  • No security concerns. trusted-github-actors.yml defaults to empty (deny-by-default) trust lists, which is the safe choice.

Test coverage

  • N/A — this is tooling/config only, no app behavior changed. PR description notes .agents/bin/test was attempted locally and failed only due to a missing generated SSR bundle in the checkout, unrelated to this change.

Overall this is low-risk, well-scoped config addition. The two inline notes are minor efficiency/consistency nits, not blockers.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
.agents/bin/test (1)

5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider forwarding "$@" for consistency with other entrypoints.

Unlike validate and lint, this script doesn't forward CLI arguments to the underlying command. Users who expect to pass specific test files (e.g., .agents/bin/test spec/models/foo_spec.rb) will be surprised. The line-range change details confirm this is intentional, but if bin/ci accepts args, forwarding them would improve uniformity across entrypoints.

♻️ Optional: forward arguments
-exec bin/conductor-exec bin/ci
+exec bin/conductor-exec bin/ci "$@"
🤖 Prompt for 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.

In @.agents/bin/test at line 5, Update the .agents/bin/test entrypoint’s exec
invocation to forward all received CLI arguments to bin/ci via "$@". Preserve
the existing command and execution behavior while allowing callers to target
specific test files or options.
🤖 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 @.agents/bin/build:
- Around line 6-8: Update the build script to remove the standalone
react_on_rails:generate_packs and yarn res:build commands, leaving the yarn
build:test "$@" invocation to perform those steps without duplication.

---

Nitpick comments:
In @.agents/bin/test:
- Line 5: Update the .agents/bin/test entrypoint’s exec invocation to forward
all received CLI arguments to bin/ci via "$@". Preserve the existing command and
execution behavior while allowing callers to target specific test files or
options.
🪄 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

Run ID: 7505ee2b-c169-48aa-b844-103125e6a025

📥 Commits

Reviewing files that changed from the base of the PR and between cd56081 and cc4dea0.

📒 Files selected for processing (6)
  • .agents/bin/README.md
  • .agents/bin/build
  • .agents/bin/lint
  • .agents/bin/setup
  • .agents/bin/test
  • .agents/bin/validate
✅ Files skipped from review due to trivial changes (1)
  • .agents/bin/README.md

Comment thread .agents/bin/build Outdated
@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: rwrt-qa-anchor; read-only isolated verification; private coordination: not applicable (coordination_backend: n/a).
  • Scope checked: PR chore: add agent workflow seam #778 command seam (setup, build, lint, test, and validate) against trusted config/ci.rb at the current head.
  • Tested at: cc4dea03df8616cef69a9fdabcf2604092f4f9df.
  • Automated checks: seam doctor passed; wrapper syntax and executable-bit checks passed; each wrapper’s command contract matches the established project CI flow.
  • Manual checks: confirmed test delegates to bin/ci, whose configured sequence includes setup, Ruby/JS lint, test-asset build, RSpec, and Jest.
  • Findings: none. The prior missing test-preparation and JS-coverage concerns are fixed in cc4dea03.
  • QA required: yes.
  • QA required rationale: this changes developer-workflow commands and agent instruction surfaces.
  • QA lane status: satisfied.
  • Release-blocking status: clear from QA; hosted current-head checks remain separately pending.
  • Process-gap disposition: checklist+replay.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc4dea03df

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

Comment thread .agents/bin/test Outdated
Comment thread .agents/bin/test Outdated
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Summary

This PR adds a small, self-contained "agent workflow seam": .agents/agent-workflow.yml (policy metadata), .agents/trusted-github-actors.yml (empty allowlists — safe, deny-by-default), and .agents/bin/{setup,validate,test,lint,build} wrapper scripts, plus doc updates in AGENTS.md and .agents/bin/README.md. It's auto-generated by agent-workflow-seam-doctor and lets shared ShakaCode agent workflows discover this repo's commands without duplicating logic.

Verified

  • All commands referenced by the wrapper scripts (bin/setup, bin/rubocop, bin/ci, bin/conductor-exec) exist and are executable.
  • .agents/bin/buildyarn build:test already runs res:build (ReScript) and react_on_rails:generate_packs internally, so the README's phrasing ("generate React on Rails packs, build ReScript, then yarn build:test") is just descriptive, not indicating separate/missing steps — no discrepancy there.
  • Wrapper scripts use cd/exec safely with quoted "$@" — no shell-injection concerns.
  • trusted-github-actors.yml defaults to empty lists, which is the conservative/secure default.

Findings

  • Minor inconsistency (inline comment on .agents/bin/test): unlike its sibling scripts, .agents/bin/test doesn't forward "$@" to bin/ci. Harmless today since bin/ci ignores ARGV, but worth making consistent or documenting intentionally.
  • Cosmetic: blank-line formatting after the cd line is inconsistent across the generated scripts (build/setup/validate have a blank line before exec, lint/test don't). Not worth blocking on since it's generator output, but flagging for awareness.

Risk

Low — this PR only adds metadata/config and thin wrapper scripts; it doesn't touch application code, and the author's own validation (.agents/bin/validate) passed with no RuboCop offenses. The noted 47 failing specs are pre-existing/environmental (missing ssr-generated/server-bundle.js), not caused by this change.

@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: rwrt-qa-anchor; read-only isolated verification; private coordination: not applicable (coordination_backend: n/a).
  • Scope checked: PR chore: add agent workflow seam #778’s complete command seam, including the corrected non-duplicating build wrapper, against trusted config/ci.rb at the current head.
  • Tested at: 2155fe28b095f5cb59cab04e7f25d3cbeccfb490.
  • Automated checks: seam doctor passed; all five wrappers passed syntax and executable-bit checks; command routing matches project entrypoints.
  • Manual checks: build delegates only to yarn build:test; test delegates to bin/ci, whose configured sequence includes setup, Ruby/JS lint, asset build, RSpec, and Jest.
  • Findings: none.
  • QA required: yes.
  • QA required rationale: this changes developer-workflow commands and agent instruction surfaces.
  • QA lane status: satisfied.
  • Release-blocking status: clear from QA; hosted current-head checks remain separately pending.
  • Process-gap disposition: checklist+replay.

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: full history through current head 2155fe28b095f5cb59cab04e7f25d3cbeccfb490.

Mattered

  • Test preparation and JS coverage: addressed in cc4dea03 by routing the test seam through the repository’s established full bin/ci path and exposing setup, lint, and build capabilities.
  • Duplicate build preparation: addressed in 2155fe28; the build seam now delegates only to yarn build:test.

Optional

  • Selective test arguments: declined. bin/ci has no argument-dispatch contract, so forwarding arguments would silently discard them and advertise unsupported behavior.

Skipped

  • Duplicate inline reports of the missing preparation path were resolved with the same cc4dea03 fix.
  • Bot review summaries were handled through the linked inline-thread dispositions.

All current review threads were replied to and resolved. The final-head QA evidence is recorded separately in the PR discussion.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: rwrt-qa-anchor; read-only isolated verification; private coordination: not applicable (coordination_backend: n/a).
  • Scope checked: PR chore: add agent workflow seam #778’s complete command seam and managed renderer lifecycle in the test wrapper at the current head.
  • Tested at: 74056737d3c172e53ac69735b860ed950634af48.
  • Automated checks: seam doctor passed; all wrappers passed syntax and executable-bit checks; lifecycle assertions verify reuse of an existing renderer, bounded readiness, early-exit diagnostics, and cleanup only for the owned PID.
  • Manual checks: the renderer probe matches configured localhost port 3800; the test wrapper invokes the established conductor-wrapped CI entrypoint.
  • Findings: none.
  • QA required: yes.
  • QA required rationale: this changes developer-workflow commands and agent instruction surfaces.
  • QA lane status: satisfied.
  • Release-blocking status: clear from QA; hosted current-head checks remain separately pending.
  • Process-gap disposition: checklist+replay.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
.agents/bin/test (1)

9-18: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider SIGKILL escalation in cleanup.

cleanup_renderer sends SIGTERM and then waits indefinitely. If the renderer doesn't shut down gracefully, the script hangs on exit. A short timeout followed by SIGKILL would prevent this.

♻️ Optional refactor
 cleanup_renderer() {
   local status=$?

   if [[ "$renderer_started" == true ]] && kill -0 "$renderer_pid" 2>/dev/null; then
     kill "$renderer_pid" 2>/dev/null || true
-    wait "$renderer_pid" 2>/dev/null || true
+    local i
+    for i in $(seq 1 10); do
+      kill -0 "$renderer_pid" 2>/dev/null || break
+      sleep 0.5
+    done
+    if kill -0 "$renderer_pid" 2>/dev/null; then
+      kill -9 "$renderer_pid" 2>/dev/null || true
+      wait "$renderer_pid" 2>/dev/null || true
+    fi
   fi

   exit "$status"
 }
🤖 Prompt for 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.

In @.agents/bin/test around lines 9 - 18, Update cleanup_renderer to wait only
briefly after sending SIGTERM, then send SIGKILL to renderer_pid if the process
is still running before waiting for it. Preserve the existing status capture and
cleanup behavior, including avoiding errors when the renderer has already
exited.
🤖 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 @.agents/bin/test:
- Around line 22-45: Update the renderer startup and readiness checks in the
test script to derive the port from RENDERER_PORT, defaulting to 3800 when
unset, instead of hardcoding 3800. Use the same derived port for all nc checks
and status messages, while preserving the existing renderer startup and timeout
behavior.

---

Nitpick comments:
In @.agents/bin/test:
- Around line 9-18: Update cleanup_renderer to wait only briefly after sending
SIGTERM, then send SIGKILL to renderer_pid if the process is still running
before waiting for it. Preserve the existing status capture and cleanup
behavior, including avoiding errors when the renderer has already exited.
🪄 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

Run ID: 7eda5324-0e73-4160-a8ed-c742334e2600

📥 Commits

Reviewing files that changed from the base of the PR and between 2155fe2 and 7405673.

📒 Files selected for processing (1)
  • .agents/bin/test

Comment thread .agents/bin/test Outdated
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review

Overview

Adds a "seam" so shared ShakaCode agent-workflow skills can discover this repo's commands: .agents/agent-workflow.yml (policy/metadata), .agents/trusted-github-actors.yml (empty allowlists — safe default-deny), .agents/bin/{setup,validate,test,lint,build} wrapper scripts, a .agents/bin/README.md reference table, and an AGENTS.md doc addition. Pure additive infra/config change, no application code touched.

Code quality

  • The thin wrappers (setup, validate, lint, build) are consistent, set -euo pipefail, cd to repo root, and exec ... "$@" to forward args and propagate signals/exit codes cleanly. Nice and uniform.
  • .agents/bin/test is more involved: it starts renderer/node-renderer.js if port 3800 isn't already listening, waits up to 30s for readiness, and cleans it up via an EXIT trap. This logic closely mirrors the existing "Start Node renderer for SSR" step in .github/workflows/rspec_test.yml (lines 88-105) — good that it's consistent with the established pattern, but it's now duplicated in two places. Worth considering extracting the wait-for-renderer logic into a small shared script both the workflow and .agents/bin/test can call, so the two don't drift out of sync later.
  • .agents/bin/test correctly avoids exec-ing into the final bin/conductor-exec bin/ci (unlike the sibling scripts) — using exec there would replace the process image and the EXIT trap/cleanup would never run. Good attention to detail.
  • Minor inconsistency: unlike the other four wrappers, .agents/bin/test never forwards "$@" to bin/ci. In practice this doesn't currently matter since bin/ci (config/ci.rb) ignores ARGV entirely and always runs the full step list, but it does mean this wrapper can't support a common agent-workflow use case of running a single spec/file for fast iteration. Worth a comment noting that test always runs the full suite, or consider adding arg passthrough support if per-file test runs are ever needed.

Security

  • .agents/trusted-github-actors.yml ships with empty trusted_users/trusted_bots/trusted_metadata_bots/trusted_teams lists — a safe default-deny posture rather than pre-populating trust. No concerns.
  • No secrets or credentials introduced. The renderer password fallback (local-dev-renderer-password) used implicitly by .agents/bin/test when RENDERER_PASSWORD is unset already matches the existing dev/test default in config/initializers/react_on_rails_pro.rb and renderer/node-renderer.js, so no auth mismatch.

Testing

  • PR description notes .agents/bin/test was attempted but 47 specs failed locally due to a missing generated SSR bundle (ssr-generated/server-bundle.js) in the checkout — this is a pre-existing environment/build-artifact issue unrelated to this change, not a regression introduced by it. Worth double-checking in CI (where the build step generates that bundle first) rather than relying solely on the local attempt.
  • This is infra/tooling metadata with no app behavior change, so no spec coverage is expected or needed here.

Suggestions

  1. Consider deduplicating the renderer-wait logic between .github/workflows/rspec_test.yml and .agents/bin/test.
  2. Document (in .agents/bin/README.md or inline) that test always runs the full CI step list and doesn't support filtering to a single spec.

Overall this is a low-risk, well-structured addition — no correctness or security blockers found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74056737d3

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

Comment thread .agents/bin/test
Comment thread .agents/bin/test
Comment thread .agents/bin/README.md
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review: chore: add agent workflow seam

Overview

This PR adds a self-describing "agent workflow seam" so shared/portable agent tooling (from shakacode/agent-workflows) can discover this repo's commands without embedding repo-specific logic:

  • .agents/agent-workflow.yml — policy metadata (base branch, mostly n/a placeholders for unused features)
  • .agents/trusted-github-actors.yml — empty trust allowlists (safe default: deny by default)
  • .agents/bin/{setup,validate,test,lint,build} — thin wrapper scripts delegating to existing bin/* / yarn scripts via bin/conductor-exec
  • .agents/bin/README.md — documents the command table
  • AGENTS.md — new "Agent Workflow Configuration" section pointing at the above

This is infra/metadata only — no application code paths are touched, and the scope is well contained.

Code quality

  • The wrapper scripts (setup, validate, lint, build) are simple, consistent, and correctly cd to the repo root before delegating via exec, forwarding "$@". Good use of set -euo pipefail.
  • .agents/bin/test is the most complex piece — it manages a Node renderer's lifecycle (start it if not already listening, wait for readiness, clean up via an EXIT trap). The trap correctly preserves the original exit status and only kills the renderer if this script started it (so it won't kill a pre-existing renderer it merely reused). This is solid defensive scripting.
  • Executable bits are set correctly on the new scripts (100755) vs. the docs/config files (100644).

Issues found (posted inline)

  1. .agents/bin/test — the readiness probe depends on the nc binary. If it's missing, the failure mode is a misleading "renderer failed to start within 30 seconds" after burning the full retry loop, rather than a clear "nc not found" error. Worth a fast-fail check or a portable fallback (e.g. /dev/tcp).
  2. .agents/bin/README.md — the build row's "This repo runs" description lists steps (generate packs, build ReScript) that aren't actually invoked by .agents/bin/build itself — they happen internally inside yarn build:test. Minor doc inconsistency vs. the other table rows, which show the literal command.

Other notes (not blocking)

  • Backgrounding node renderer/node-renderer.js through bin/conductor-exec (which execs into mise exec -- ... when mise is present) relies on mise exec replacing itself via exec so that $!/kill target the real Node process rather than an orphaned wrapper. Worth a quick sanity check in a real mise environment if not already verified, since this determines whether cleanup actually works.
  • No test coverage added for the new bash scripts, which is reasonable given they're thin wrappers around already-tested commands (bin/ci, bin/rubocop, bin/setup).
  • No security concerns: the trusted-actor allowlists default to empty/deny, and no secrets or credentials are introduced.

Overall this is a low-risk, well-scoped addition. The two inline notes are minor polish items, not blockers.

@justin808

Copy link
Copy Markdown
Member Author

QA Evidence

  • QA lane: rwrt-qa-anchor; read-only isolated verification; private coordination: not applicable (coordination_backend: n/a).
  • Scope checked: PR chore: add agent workflow seam #778’s complete command seam, test setup ordering, renderer lifecycle, and renderer port/URL alignment at the current head.
  • Tested at: 4ee4c0752e389807a3538cb5454fd4366da77611.
  • Automated checks: seam doctor passed; all wrappers passed syntax and executable-bit checks; RENDERER_PORT=3811 derives RENDERER_URL=http://localhost:3811; an explicit URL is preserved.
  • Manual checks: setup precedes renderer startup; the wrapper reuses existing renderers, probes the selected port, records only its own PID, and cleans up only that owned process.
  • Findings: none.
  • QA required: yes.
  • QA required rationale: this changes developer-workflow commands and agent instruction surfaces.
  • QA lane status: satisfied.
  • Release-blocking status: clear from QA; hosted current-head checks remain separately pending.
  • Process-gap disposition: checklist+replay.

Comment thread .agents/bin/test
Comment thread .agents/bin/test
Comment thread .agents/trusted-github-actors.yml
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review

Overview

Adds a shared "agent workflow seam" so external agent-workflow tooling can discover this repo's setup/validate/test/lint/build commands and policy without hardcoding logic per-repo: .agents/agent-workflow.yml (policy), .agents/trusted-github-actors.yml (trust config), .agents/bin/{setup,validate,test,lint,build} (command wrappers), .agents/bin/README.md, and an AGENTS.md doc addition. Purely additive — no existing files' behavior changes.

Code quality

  • The wrapper scripts are small, consistently structured (set -euo pipefail, resolve repo root, delegate through bin/conductor-exec), and match the existing bin/conductor-exec convention documented in AGENTS.md. Good consistency.
  • .agents/bin/test is meaningfully more complex than the others because it manages the Node SSR renderer's lifecycle (start/health-check/cleanup via an EXIT trap). The trap logic itself (capturing $?, conditionally killing, re-raising the exit status) is correctly written bash.

Issues found (posted inline)

  1. Duplicate setup invocation (.agents/bin/test): it explicitly runs bin/conductor-exec bin/setup --skip-server, then calls bin/conductor-exec bin/ci, whose first step (config/ci.rb) already runs the exact same command. Every test run pays for setup twice.
  2. Possible orphaned renderer process: renderer_pid=$! is captured from the bin/conductor-exec ... & backgrounded call. Since bin/conductor-exec does exec mise exec -- "$@", the PID stays valid across that specific exec, but if mise exec itself forks rather than execve-replaces for the final node command, kill "$renderer_pid" would kill the wrapper but orphan the actual renderer process — leaving port 3800 occupied for later runs. Worth verifying, or killing the process group instead of a single PID for safety.
  3. Empty trust lists in .agents/trusted-github-actors.yml (deny-by-default) — likely intentional, but flagging in case any shared workflow expects default trusted bots/teams to be pre-populated.

Other notes

  • Health-check depends on nc being available in whatever environment runs these scripts; not new risk, just an assumption worth being aware of.
  • No security concerns beyond Integrate npm bootstrap-sass; Remove bootstrap-sass gem; Update README #3 above — no secrets, no untrusted input execution, scripts only wrap existing local commands.
  • Test coverage: N/A (shell/config wrappers); PR description notes .agents/bin/test itself couldn't be fully validated in this environment due to a missing generated SSR bundle, unrelated to this PR's code.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ee4c0752e

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

Comment thread .agents/bin/validate
@justin808
justin808 merged commit adcd64a into master Jul 11, 2026
58 checks passed
@justin808
justin808 deleted the jg-codex/agent-workflows-seam-clean branch July 11, 2026 22:56
@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown

✅ Review App Deleted

Review app for PR #778 is deleted

🎮 Control Plane Console
📋 View Workflow Logs

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant