Keep generated simulation seams current#103
Conversation
WalkthroughThe simulation template adds config-only and seam-guard validation, merge-base-aware CI routing, full-history checkout, expanded command documentation, and a comprehensive isolated-repository test suite. ChangesSimulation Template Enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant config_check
participant validate
participant test
CI->>config_check: classify repository changes
config_check-->>CI: CONFIG_ONLY, TASK_ONLY, or NO_CHANGES
CI->>validate: validate task changes
CI->>test: run full test suite for no changes
validate-->>CI: execute matching task test
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ReviewOverviewThis PR keeps the simulation template's generated "seams" durable: it aligns the Code quality / correctness
Test coverage
SecurityNo injection or path-traversal concerns spotted — paths are hardcoded/joined via PerformanceNegligible — this only runs on config-only diffs during Overall: small, well-scoped, well-tested change. Nice work. |
![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: 9aa3bf204e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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: 1d1fb52d4f
ℹ️ 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".
|
Review: Keep generated simulation seams current (#103) Overview What's solid
Issues found (posted inline)
Maintainability note (not inline) Nit (no action needed) |
Review: Keep generated simulation seams current (#103)OverviewAdds a Critical: incomplete trust chain (see inline comments)The hash-pinning is meant to stop a PR (in this simulation, an AI worker) from tampering with the enforcement scripts, but Other findings
Positives
Test coverageGood coverage of the intended success/failure paths for |
Review summaryWhat this PR does: Adds a two-tier "seam guard" mechanism to the simulation template (
Overall: This is a solid, thoughtful security design. The core idea — that a Findings (posted inline)
Other notes (no action needed)
|
Review summaryThis PR replaces the previous all-or-nothing gate (any non-task-file change made Code quality
Behavior / risk
Security
Tests
Overall: solid, well-tested change with a sensible security model. Findings above are mostly maintainability/documentation nits rather than correctness bugs. |
Review summaryThis PR adds a config-only maintenance path ( I left 3 inline comments on concrete issues:
Other notes:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d46a257bd2
ℹ️ 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".
Review: Keep generated simulation seams current (#103)OverviewThis PR adds a two-layer guard to the Strengths
Issues raised inline
Minor
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02d5e89aa5
ℹ️ 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".
|
|
||
| # Keep these duplicated checks aligned with config-check. Duplication is | ||
| # intentional: this base-trusted guard must never require candidate code. | ||
| agents = head_file(root, head_commit, "AGENTS.md") |
There was a problem hiding this comment.
When a configuration-only PR is based on an older revision of the guarded contract, the diff correctly ignores base-only changes, but these checks still read every required file from the stale head commit. For example, after the canonical AGENTS.md pointer is updated on the base branch, an older policy-only PR is rejected for retaining the previous pointer even though merging it would preserve the valid base version. Build or inspect the prospective merge tree, or read unchanged configuration files from the base commit, so stale configuration branches are evaluated as they would actually merge.
Useful? React with 👍 / 👎.
| with: | ||
| ref: ${{ github.event.pull_request.base.sha }} | ||
| fetch-depth: 0 | ||
| persist-credentials: false |
There was a problem hiding this comment.
persist-credentials: false on this checkout means no auth token is left in the local git config. The run: step below then does git fetch --no-tags origin "+refs/pull/${PR_NUMBER}/head:..." against origin — on a private repository this fetch will fail (401/403, "repository not found") because there's no credential available for the anonymous HTTPS remote. This works fine for public repos, but since this template is meant to be reused broadly ("future reseeds"), it's worth confirming target repos are always public, or otherwise passing a token explicitly to the fetch (e.g. via git -c http.extraheader=... scoped to just this command) rather than relying on persisted checkout credentials.
ReviewOverviewThis PR adds a two-tier CI security model to the simulation template used to seed generated agent-eval repos:
Strengths
Issues
Test coverageThe new Security implicationsNo command injection or unsafe |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sim/test/template_test.rb (1)
335-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelper naming:
validatewrapsci, not.agents/bin/validate.The
validateprivate helper actually invokes.agents/bin/ci(line 338), whileconfig_check/seam_guardhelpers invoke their namesake binaries directly. This is intentional (testing the full ci-routing gate), but the name can mislead readers into thinking it calls.agents/bin/validatedirectly.🤖 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 `@sim/test/template_test.rb` around lines 335 - 341, Rename the private helper validate to a name that clearly indicates it invokes the full .agents/bin/ci routing gate, and update all references to the helper accordingly. Leave the Open3 invocation and the config_check/seam_guard helpers unchanged.sim/template/.agents/bin/ci (1)
16-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated base_ref fallback logic between
ciandvalidate, without a sync-comment. Both scripts implement the identicalAGENT_SIM_BASE_REF→GITHUB_BASE_REFfetch →main/origin/mainfallback chain; unlike the other intentional duplications in this PR (e.g., config-check/seam-guard canonical-pointer checks), neither copy carries a "keep these aligned" comment, so a future edit to one could silently diverge from the other.
sim/template/.agents/bin/ci#L16-L26: extract the fallback chain into a small sourced helper shared withvalidate, or add a "keep aligned with validate" comment.sim/template/.agents/bin/validate#L6-L16: extract into the same shared helper, or add a "keep aligned with ci" comment.🤖 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 `@sim/template/.agents/bin/ci` around lines 16 - 26, Keep the duplicated base_ref fallback logic in sim/template/.agents/bin/ci lines 16-26 and sim/template/.agents/bin/validate lines 6-16 synchronized by either extracting it into one shared sourced helper used by both scripts, or adding explicit “keep aligned” comments to both copies; do not leave either site without synchronization guidance.
🤖 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 `@sim/test/template_test.rb`:
- Around line 208-214: The test_validate_rejects_invalid_validator_syntax
currently triggers the validator hash-mismatch check rather than syntax
validation. Retarget it to a non-hash-pinned script, following
test_config_check_rejects_invalid_test_script_syntax, so the appended invalid
syntax reaches the bash -n branch; alternatively, rename the test and assert the
hash-mismatch message.
---
Nitpick comments:
In `@sim/template/.agents/bin/ci`:
- Around line 16-26: Keep the duplicated base_ref fallback logic in
sim/template/.agents/bin/ci lines 16-26 and sim/template/.agents/bin/validate
lines 6-16 synchronized by either extracting it into one shared sourced helper
used by both scripts, or adding explicit “keep aligned” comments to both copies;
do not leave either site without synchronization guidance.
In `@sim/test/template_test.rb`:
- Around line 335-341: Rename the private helper validate to a name that clearly
indicates it invokes the full .agents/bin/ci routing gate, and update all
references to the helper accordingly. Leave the Open3 invocation and the
config_check/seam_guard helpers unchanged.
🪄 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: 2ba33e5f-36c6-4180-a897-326ae568571e
📒 Files selected for processing (10)
.agents/bin/testsim/template/.agents/bin/README.mdsim/template/.agents/bin/cisim/template/.agents/bin/config-checksim/template/.agents/bin/seam-guardsim/template/.agents/bin/validatesim/template/.github/workflows/ci.ymlsim/template/.github/workflows/seam-guard.ymlsim/template/AGENTS.mdsim/test/template_test.rb
| def test_validate_rejects_invalid_validator_syntax | ||
| File.open(File.join(@repo, ".agents/bin/validate"), "a") { |file| file << "\nif\n" } | ||
|
|
||
| _out, _err, status = validate | ||
|
|
||
| refute status.success? | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test doesn't actually exercise syntax validation — it hits the validator hash-pin instead.
config-check's validator SHA256 check runs unconditionally and precedes the bash -n syntax check, so appending if\n to .agents/bin/validate will always fail via "Simulation validator does not match the checked contract." (already covered by test_ci_rejects_malicious_validator_that_exits_early), never reaching the syntax-check branch this test's name implies. The test still passes because it only asserts refute status.success?.
Either assert on the actual message produced (hash-mismatch), or retarget this test at a non-hash-pinned script (as test_config_check_rejects_invalid_test_script_syntax does for .agents/bin/test) to genuinely cover the syntax-check path.
🤖 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 `@sim/test/template_test.rb` around lines 208 - 214, The
test_validate_rejects_invalid_validator_syntax currently triggers the validator
hash-mismatch check rather than syntax validation. Retarget it to a
non-hash-pinned script, following
test_config_check_rejects_invalid_test_script_syntax, so the appended invalid
syntax reaches the bash -n branch; alternatively, rename the test and assert the
hash-mismatch message.
1 participant
Update the simulation template so future reseeds preserve the canonical Agent Workflow Configuration pointer and checked config-only maintenance path. The config checker validates allowed paths, shell and Ruby syntax, validator dispatch, canonical pointer text, policy YAML, and the documented command contract. Task/config mixtures and unrelated paths remain rejected.\n\nValidation:\n- focused template tests: 10 runs, 28 assertions, 0 failures\n- generated template seam doctor against merged agent-workflows source: PASS\n- RuboCop: 36 files, 0 offenses\n- clean-environment repository test wrapper: all suites passed
Seam-guard bootstrap boundary
The generated
pull_request_targetguard is base-trusted and begins protecting a generated repository only after that repository merges its first adoption. That first adoption is an explicit bootstrap boundary; ordinary head-controlled CI is not sufficient proof for it.Summary by CodeRabbit
New Features
Documentation
Tests