Conversation
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
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 |
justin808
added
the
agent-claimed
Independent exact-head check — CLEANReviewed issue #258 and the exact six-file diff at required head Verdict: CLEAN for the lane implementation: no Focused evidence:
Concern dispositions:
Live GitHub currently reports 0 review threads and 0 review objects (only CodeRabbit's draft-skip comment), despite the checker brief referring to two unresolved threads. I did not resolve/dismiss anything; the two scoped concerns above are the durable dispositions. Hosted-state separation:
Next writer action: after #662 and the #695/#696 path owners settle, reconcile/replay those three overlapping files onto a fresh head, run full hosted Validate plus Claude review, then request a new exact-head check. pending_maintainer_action:
required: true
state: waiting_code_change
owner: author
action: 'Reconcile #662/#695/#696, push the integrated head, rerun hosted checks, and request exact-head recheck'
reason: 'Current implementation is clean, but exact-head CI is NOT_READY and overlapping path owners remain'
blocks_merge: true
evidence:
pr: 677
head_sha: '6b32fc064040277cd89ea002257d8b8cd095b11c'
review_decision: null
maintainer_approval_comment: false
ledger_complete_allowed: 'UNKNOWN'
ci_readiness: NOT_READY |
justin808
added
agent-claimed
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the drift-gate helper and the seam-doctor validation it pairs with. The path-traversal/encoding validation in One thing worth surfacing before this merges: The drift gate has no automated CI enforcement anywhere — it's entirely agent-compliance-based. The PR's stated motivation is that "generated templates can drift from hand-maintained mirrors without any CI signal," but the fix as implemented doesn't add a CI signal either:
So for a consuming repo that declares Is that gap intentional for this draft (e.g. wiring into hosted CI is planned as follow-up), or should Also left an inline note about validation-logic duplication between |
justin808
added
agent-claimed
|
This PR intentionally stops at the portable runtime gate, seam-doctor policy validation, and local |
Address-review summaryScan scope: full PR history on 2026-09-02 (no prior workflow summary comment). Detailed review outcomesMattered
Optional
Skipped
Hosted checks on Next default scan starts after this comment. Say |
justin808
removed
the
agent-claimed
Review summaryReviewed the new Two things worth a look, posted as inline comments:
Neither is a blocking correctness bug in the tested paths (all the tests I checked pass green given the logic as written), but both seem worth addressing before merge or as fast-follow. |
|
Claude finished @justin808's task in 1m 24s —— View job Address-review summaryScan scope: re-invocation on the same comment (no new review activity found). Detailed review outcomesMattered
Optional
Skipped
Hosted checks on Next default scan starts after this comment. Say |
justin808
added
the
agent-claimed
|
Reconciled to current main at |
justin808
removed
the
agent-claimed
|
Review summary Reviewed the generated-artifact drift gate (seam-doctor policy validation, the new Overall the design is solid: undeclared repos stay silent, validation fails closed before path evaluation runs, the helper is dependency-free and its schema/path-validation logic is a faithful mirror of Bug: stale "Instructions step" cross-references in Before this PR, "Decide the required verification set..." (the explanation for why
Both now point at the wrong instruction ("Run the generated-artifact drift gate" instead of the docs-only Minor: inconsistent --end-of-options guarding in generated-artifact-drift Left as an inline comment on Other notes (non-blocking)
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: b3b939425e
ℹ️ 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 summaryReviewed the generated-artifact drift gate addition (seam-doctor validation + the new Left 4 inline comments:
No security or performance concerns beyond what's noted above — the helper only compares changed-path sets and never reads declared file contents, so path validation is defense-in-depth rather than exploitable today. |
justin808
added
the
agent-claimed
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4ace90e4
ℹ️ 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".
justin808
removed
the
agent-claimed
| declarations.each_with_index do |declaration, index| | ||
| next unless changed_files.include?(declaration["source"]) | ||
|
|
||
| unless declaration["golden_test"].is_a?(String) && !declaration["golden_test"].strip.empty? |
There was a problem hiding this comment.
Correctness: whitespace-only golden_test is validated as "declared" but rejected as "not declared" at runtime.
path_errors/generated_artifact_path_errors (both here and in bin/agent-workflow-seam-doctor) only check !value.empty?, so a golden_test value of " " (a single space) passes seam-doctor validation as a structurally valid declaration. But this line uses !declaration["golden_test"].strip.empty?, which is stricter — a whitespace-only value fails this check and falls into the "golden_test is not declared" failure branch, even though the declaration was declared and passed validation.
Concretely: a declaration with golden_test: " " is accepted by agent-workflow-seam-doctor as valid, but when its source changes, this gate reports "generated_artifacts[N].golden_test is not declared" — a confusing, incorrect message for a value that a "keep these checks aligned" comment (line 119-120) claims stays in sync with the doctor's validation.
Consider using the same .empty? (non-stripped) check here, or tightening path_errors in both places to reject whitespace-only paths, so the two "aligned" implementations agree.
| end | ||
| end | ||
|
|
||
| def duplicate_key_errors(node, path, errors) |
There was a problem hiding this comment.
Reuse: this duplicate-key walker reimplements AutonomousMergePolicy.duplicate_key_errors from scratch.
bin/agent_doctor/autonomous_merge_policy_yaml.rb already defines an identical recursive duplicate-key checker (duplicate_key_errors/walk_for_duplicate_keys), and bin/agent-workflow-seam-doctor calls it via AutonomousMergePolicy.duplicate_key_errors(yaml) for this very same generated_artifacts policy (see generated_artifacts_policy_issues). This file re-derives the same algorithm (and, further down, declaration_errors/path_errors duplicate generated_artifact_declaration_errors/generated_artifact_path_errors from bin/agent-workflow-seam-doctor almost line-for-line) instead of requiring the shared module.
Other skill scripts in this repo already reach across directories for this exact module, e.g. skills/pr-batch/bin/autonomous-merge-contract-test.rb and skills/pr-batch/bin/current-integration-evidence-test.rb do require_relative "../../../bin/agent_doctor/autonomous_merge_policy", so sharing here would follow an established convention rather than introduce a new one.
The comment above (lines 119-120) explicitly flags the duplication risk ("stays dependency-free by mirroring the same validation rules here") — but that's exactly the kind of drift this PR's own feature is designed to catch. If the schema ever changes in one copy and not the other, the doctor and the runtime gate would silently disagree on what's valid.
| entry's `source` changes. When the source changes, a declared `golden_test` must also resolve to an existing repository | ||
| file beneath the selected root. | ||
|
|
||
| Resolve `VERIFY_SKILL_DIR` from an explicit environment value, the loaded skill's base directory, or the repo-local |
There was a problem hiding this comment.
Doc/procedure mismatch: the described 3-way VERIFY_SKILL_DIR fallback isn't what the snippet below does.
This sentence says to resolve VERIFY_SKILL_DIR from "an explicit environment value, the loaded skill's base directory, or the repo-local .agents/skills/verify copy" (three fallbacks), but the bash snippet immediately below only implements two: VERIFY_SKILL_DIR="${VERIFY_SKILL_DIR:-.agents/skills/verify}" uses an explicit env value if set, otherwise jumps straight to the repo-local copy — it never tries "the loaded skill's base directory" in between.
For an agent that copies the bash block verbatim (rather than parsing the surrounding prose), this means: if the skill is loaded from somewhere other than .agents/skills/verify (e.g. this repo's own skills/verify/bin/..., or a different vendoring path) and VERIFY_SKILL_DIR isn't set, the command resolves to the wrong path and the "stop with a precise missing-helper error" fallback never triggers the intended middle case.
| issues.uniq | ||
| end | ||
|
|
||
| def generated_artifacts_policy_issues(config, yaml) |
There was a problem hiding this comment.
Rollout risk: this can flip existing downstream consumers from "clean" to "drifted".
generated_artifacts_policy_issues now unconditionally validates the shape of a generated_artifacts: key whenever it's present in .agents/agent-workflow.yml — a key that seam-doctor previously ignored entirely (no top-level-key allowlist exists). This is consumed by bin/push-downstream's fleet audit: issues = AgentWorkflowSeamDoctor.check(clone) (line ~1029) feeds directly into status: issues.empty? && changed_paths.empty? ? AUDIT_CLEAN : "drifted" (line ~1056), which gates .github/workflows/downstream-seam-audit.yml.
Any consumer repo that already has a generated_artifacts: key in a shape this PR doesn't accept (wrong type, missing mirrors, duplicate source, non-canonical path, etc. — e.g. pre-adopted ahead of this schema landing, or added by a parallel in-flight PR in the same batch) will newly fail seam-doctor and flip from clean to drifted on the next scheduled audit, even though nothing about the actual seam contract changed. Worth confirming no consumer in downstream.yml currently declares this key in an incompatible shape before merging.
| with `git status --short`, `git diff --name-only "origin/${BASE_BRANCH}...HEAD"`, and | ||
| `git diff --stat "origin/${BASE_BRANCH}...HEAD"`. | ||
| 3. Decide the required verification set that covers the changed surface area using the **Scope Guide** below. Always | ||
| 3. Run the [generated-artifact drift gate](#generated-artifact-drift-gate). Preserve any acknowledgment-required |
There was a problem hiding this comment.
The drift gate this PR adds is never actually invoked by any enforced/automated check — only by agent-followed prose.
The only place that runs skills/verify/bin/generated-artifact-drift --root ... --base-ref ... against a real diff is this natural-language /verify instruction. bin/validate only runs the helper's unit tests (ruby skills/verify/bin/generated-artifact-drift-test.rb), not the actual gate against the branch diff, and no .github/workflows/*.yml references the script at all.
Given the PR's stated goal is "no CI signal" today for template/mirror drift, this still leaves no CI signal — it just relies on an agent correctly reading and executing SKILL.md step 3 (resolving VERIFY_SKILL_DIR, running the helper, copying the advisory into evidence) before every commit. A human pushing directly, or an agent skipping/misreading step 3, or misresolving VERIFY_SKILL_DIR (silent no-op) ships the exact drift this feature is meant to catch, undetected by CI's mandatory gates (bin/validate, bin/lint, downstream-seam-audit.yml). Consider wiring the actual check into an enforced gate rather than prose-only instructions.
| end | ||
| end | ||
|
|
||
| def duplicate_key_errors(node, path, errors) |
There was a problem hiding this comment.
Reimplements an existing, already-reusable YAML duplicate-key detector instead of requiring it.
duplicate_key_errors/generated_artifact_duplicate_errors (lines 143-168) is a near-verbatim reimplementation of AutonomousMergePolicy.duplicate_key_errors/walk_for_duplicate_keys in bin/agent_doctor/autonomous_merge_policy_yaml.rb, which is already required from the identical directory depth elsewhere (e.g. skills/pr-batch/lib/autonomous_merge_decision.rb:5 does require_relative "../../../bin/agent_doctor/autonomous_merge_policy") and has zero external gem dependencies (pure stdlib Psych/YAML). The comment at line ~130 justifying the duplication as keeping the helper "dependency-free" doesn't hold up given that precedent.
Similarly, declaration_errors/path_errors (lines 171-217ish) duplicate bin/agent-workflow-seam-doctor's generated_artifact_declaration_errors/generated_artifact_path_errors almost line-for-line. Notably, within this same PR the seam-doctor calls the shared AutonomousMergePolicy.duplicate_key_errors while this sibling script reimplements the same check from scratch — an inconsistency in the PR's own approach to reuse.
Failure scenario: a future schema/edge-case fix (new allowed key, tighter path rule, YAML anchor/merge-key handling) gets applied to one copy and not the other, causing the seam doctor and drift gate to silently disagree on which generated_artifacts declarations are valid — the exact "policy drift" class of bug this feature exists to prevent, reproduced inside the tooling itself.
| errors | ||
| end | ||
|
|
||
| def generated_artifact_duplicate_errors(yaml) |
There was a problem hiding this comment.
generated_artifact_duplicate_errors re-parses the same YAML string with Psych.parse_stream(yaml) even though YAML.safe_load(yaml, aliases: false) was already run over the identical string a few lines earlier in run (line ~31) to obtain config. That's two full YAML parses of the same document on every invocation of the drift gate.
Review summaryReviewed the
Note: I also found a symlink-related bypass of the No security-critical issues beyond the symlink note, and no significant performance concerns (path-set comparisons and bounded YAML parsing only run during explicit local/CI verification). |
| ].flatten.uniq | ||
| end | ||
|
|
||
| def repository_file_beneath_root?(root, relative_path) |
There was a problem hiding this comment.
repository_file_beneath_root? re-derives symlink/path-containment safety logic (via File.realpath + Pathname#relative_path_from) that bin/agent-workflow-seam-doctor already solves elsewhere with a different technique for the same "confirm a repo-relative path is safely beneath root, rejecting symlink escapes" problem. Two independently-written, security-relevant containment checks in the same codebase risk diverging if one gets a hardening fix (e.g. after a security review) that never propagates to the other.
| 64 | ||
| end | ||
|
|
||
| def changed_files_from_git(root, base_ref) |
There was a problem hiding this comment.
changed_files_from_git spawns 4 separate git/Open3.capture3 subprocesses per invocation: a rev-parse --verify precheck plus 3 separate git diff/ls-files calls (branch diff, --cached, unstaged). The 3 diff/ls-files calls could largely be replaced by a single git status --porcelain=v1 -z --untracked-files=all, and the rev-parse --verify --quiet precheck (lines 96-98) is redundant since the subsequent git diff "#{base_ref}...HEAD" call already fails non-zero on a bad ref and gets turned into an Error with stderr text by git_changed_paths. Also note: --quiet suppresses git's own error output on a failed rev-parse, so stderr.strip at line 98 is typically empty, making the raised message "could not resolve --base-ref ...: " have an uninformative empty tail. Since /verify reruns after every fix per SKILL.md's loop instructions, this subprocess overhead is paid repeatedly per session.
| ["expected a nonempty list"] | ||
| else | ||
| declarations.each_with_index.flat_map do |declaration, index| | ||
| generated_artifact_declaration_errors(declaration, index) |
There was a problem hiding this comment.
Minor simplification: declarations.is_a?(Array) is checked twice — once inside errors.concat(if !declarations.is_a?(Array) || declarations.empty? ... end) above, and again in the following if declarations.is_a?(Array) block for the duplicate-source check. The sibling policy_errors in skills/verify/bin/generated-artifact-drift achieves the same result with a single early-return guard clause, which would be easier to follow here too.
| def run(argv) | ||
| options = { root: Dir.pwd, changed_files: [] } | ||
| parser = OptionParser.new do |opts| | ||
| opts.banner = "Usage: generated-artifact-drift [--root DIR] [--base-ref REF | --changed-file PATH ...]" |
There was a problem hiding this comment.
root = File.expand_path(options.fetch(:root)) is computed here, but changed_files_from_git is later called with the raw options.fetch(:root) (unexpanded) rather than this already-normalized root, so File.expand_path gets silently recomputed inside every downstream git call in git_changed_paths/the rev-parse precheck. Passing the already-expanded root through would avoid the redundant recomputation and the latent risk of the two expansions diverging.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc99f90421
ℹ️ 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".
| next | ||
| end | ||
| golden_test = declaration.fetch("golden_test") | ||
| unless repository_file_beneath_root?(root, golden_test) |
There was a problem hiding this comment.
When golden_test is identical to the declared source or one of its mirrors, this existence check accepts that artifact as the required test. For example, with golden_test: templates/widget.tt, changing both the source and a mirror makes the helper exit 0 silently even though no golden test exists. Validate that golden_test is distinct from the source and every mirror before accepting the declaration.
Useful? React with 👍 / 👎.
| raise Error, "could not resolve --base-ref #{base_ref.inspect}: #{stderr.strip}" unless status.success? | ||
|
|
||
| [ | ||
| git_changed_paths(root, "diff", "--name-only", "--no-renames", "-z", "#{base_ref}...HEAD", "--"), |
There was a problem hiding this comment.
Minor hardening gap: the rev-parse call two lines above guards base_ref with --end-of-options, but this diff call (and the --cached/plain diff calls below, though those don't interpolate base_ref) interpolates "#{base_ref}...HEAD" directly before the -- separator without --end-of-options. If base_ref ever began with - (e.g. --output=/some/path), git diff would parse it as an option rather than a revision — --output= in particular writes the diff to an arbitrary file instead of stdout.
In practice this is hard to trigger today: base_ref only reaches here after the earlier rev-parse --verify --quiet --end-of-options "#{base_ref}^{commit}" succeeds, which requires a literal ref/commit named that dash-prefixed string to already exist — an unusual precondition — and the only documented caller (skills/verify/SKILL.md) always passes "origin/${BASE_BRANCH}". Still, for defense-in-depth it'd be worth adding --end-of-options here too (e.g. "diff", "--name-only", "--no-renames", "-z", "--end-of-options", "#{base_ref}...HEAD", "--") so the safety property doesn't depend on that gate.
| git_changed_paths(root, "diff", "--name-only", "--no-renames", "-z", "#{base_ref}...HEAD", "--"), | ||
| git_changed_paths(root, "diff", "--name-only", "--no-renames", "-z", "--cached", "--"), | ||
| git_changed_paths(root, "diff", "--name-only", "--no-renames", "-z", "--"), | ||
| git_changed_paths(root, "ls-files", "--others", "--exclude-standard", "-z", "--") |
There was a problem hiding this comment.
Latent path-relativity bug: git ls-files --others --exclude-standard (unlike git diff --name-only, which reports paths relative to the discovered top-level regardless of -C) defaults to reporting paths relative to the -C directory itself; --full-name is needed to force top-level-relative output. If --root were ever passed as something other than the repository's true top-level (e.g. a subdirectory), this call alone would emit paths relative to that subdirectory while the three diff calls above it would still emit root-relative paths — the resulting .uniq set would silently mix two different path bases, causing untracked-file matches against declaration["source"]/mirrors to be missed or wrong.
Currently unreachable: the only documented invocation (skills/verify/SKILL.md) always passes git rev-parse --show-toplevel as --root, and the test suite only exercises --root at the same directory it runs git init in. Still, since nothing here asserts --root is the toplevel, consider adding --full-name to this call (matching the effective behavior of the other three) so the invariant holds regardless of how --root is invoked in the future.
Code ReviewReviewed the new Overall assessment: solid, well-tested addition. The design choice to compare changed-path sets rather than diffing template vs. rendered content is sound and well-justified in the PR description. Path validation ( Two minor, currently-unreachable hardening items posted as inline comments:
One structural note (not a bug): the path/schema validation logic in this new script ( No security, correctness, or performance issues found beyond the two inline items above. |
justin808
added
complexity:complexify
1 participant
Why
Generated templates can drift from hand-maintained mirrors without any CI signal. A repository needs a portable way to declare those relationships while keeping the repository's own golden-output test responsible for rendered-content correctness.
Fixes #258.
What changed
generated_artifactsseam contract with literal source, mirror, and golden-test paths./verifyhelper that blocks a changed source with no declared golden test and emits an acknowledgment-required advisory when no declared mirror changed.How to review and verify
skills/verify/bin/generated-artifact-driftfor the two decisions: the missing-golden failure and source-only advisory.Test plan
Validateon6b32fc064040277cd89ea002257d8b8cd095b11c- pending.ruby skills/verify/bin/generated-artifact-drift-test.rb- 10 runs, 127 assertions, 0 failures.ruby bin/agent-workflow-seam-doctor-test.rb- 341 runs, 2730 assertions, 0 failures.deferred_to_update_changelog.CHANGELOG.mdis unchanged.Agent details
Commands and results
ruby skills/verify/bin/generated-artifact-drift-test.rb(10 runs, 127 assertions).ruby bin/agent-workflow-seam-doctor-test.rb(341 runs, 2730 assertions).bin/agent-workflow-seam-doctor --root . --shared ..rubocop bin/agent-workflow-seam-doctor bin/agent-workflow-seam-doctor-test.rb skills/verify/bin/generated-artifact-drift skills/verify/bin/generated-artifact-drift-test.rbwith locally available RuboCop 1.90.0 (4 files, no offenses).markdownlint-cli2 skills/verify/SKILL.mdwith 0 issues.bash -n bin/validateandshellcheck bin/validate.git diff --checkandgit diff --cached --checkat the applicable pre-commit checkpoints.skills/verify/bin/generated-artifact-drift --root . --base-ref origin/main; this repository has no declaration, so the required result was silent.bin/validate, per the lane instruction to use only focused checks and take hosted Validate on the stable pushed head as full verification.Exact-head and replay evidence
db989c34d6910095a066beef07b67fb6be657c94(origin/main).6b32fc064040277cd89ea002257d8b8cd095b11c.stage-dependency-gatev1eligible; planaw-medium-wave2-20260901-plan-v1; bindingsha256:61ec491bf81b1998cac288b5fd1a3272a53b1defbd3a62e2a64407bcc703bf12; no base refresh required.AGENTS.mdandworkflows/pr-processing.mdwere reviewed. Existing consumers remain unchanged because omission is silent. Verification binds changed-path discovery to an explicit base ref and includes committed, staged, unstaged, and untracked paths; base movement requires a fresh verification replay under the existing workflow.QA Evidence
claude-m1-issue258-checker; worktree/claim statusUNKNOWNpending independent checker pickup.6b32fc064040277cd89ea002257d8b8cd095b11c.Coordination and reviewer telemetry
aw-medium-wave2-20260901/issue258-generated-drift-gate.aw-258-m5-generated-drift.codex-m5-issue258-generated-drift-gate; checker:claude-m1-issue258-checker.ask; this draft will not be merged by the maker.Decision log
golden_testmake the seam structurally invalid?bin/validatewas outside the initial path list.Merge confidence
Blocked pending hosted Validate and independent checker evidence. Merge authority remains
ask.Audit receipts
No completed-batch audit receipt yet; this is an early draft PR.