Skip to content

Commit bf03937

Browse files
justin808justin808
authored
Fix autonomous merge review findings (#267)
* Fix autonomous merge review findings * Reject nested autonomous merge placeholders * Tighten autonomous merge contract tests * Harden autonomous merge input validation * Restore legacy seam placeholder checks
1 parent 984f5b4 commit bf03937

7 files changed

Lines changed: 478 additions & 57 deletions

‎bin/agent-workflow-seam-doctor‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ module AgentWorkflowSeamDoctor
160160
)
161161
[^>\n]*>
162162
}ix
163+
FULL_STRING_ANGLE_PLACEHOLDER = /\A<[^<>\n]*[^\s<>\n][^<>\n]*>\z/
163164
CI_PARITY_KEYWORDS = /(?:CI\ parity\ (?:environment|command)|parity\ environment|runner\ image|reproduction\ guide)/
164165
CI_PARITY_SEAM_PLACEHOLDER = /<(?=[^>\n]*#{CI_PARITY_KEYWORDS.source}\b(?!\s*(?:[[:alnum:]_-]+\s*)*:\s*[^>\s]))[^>\n]*>/i
165166
CI_PARITY_EXECUTABLE_PLACEHOLDER = /<[^>\n]*#{CI_PARITY_KEYWORDS.source}\b[^>\n]*>/i
@@ -1659,20 +1660,26 @@ module AgentWorkflowSeamDoctor
16591660
REQUIRED_POLICY_KEYS.each do |key|
16601661
if !config.key?(key)
16611662
issues << "missing policy key: #{key}"
1662-
elsif unresolved_policy_value?(config[key])
1663+
elsif unresolved_policy_value?(config[key], placeholder_mode: :legacy)
16631664
issues << "unresolved policy value for key: #{key}"
16641665
end
16651666
end
16661667

16671668
config.each do |key, value|
16681669
next if REQUIRED_POLICY_KEYS.include?(key)
1669-
next unless unresolved_policy_value?(value)
1670+
1671+
autonomous_merge = key == "autonomous_merge"
1672+
next unless unresolved_policy_value?(
1673+
value,
1674+
empty_collections_unresolved: !autonomous_merge,
1675+
placeholder_mode: autonomous_merge ? :full_string : :legacy
1676+
)
16701677

16711678
issues << "unresolved policy value for key: #{key}"
16721679
end
16731680

16741681
if config.key?(REPO_PREFIX_POLICY_KEY) &&
1675-
!unresolved_policy_value?(config[REPO_PREFIX_POLICY_KEY]) &&
1682+
!unresolved_policy_value?(config[REPO_PREFIX_POLICY_KEY], placeholder_mode: :legacy) &&
16761683
(!config[REPO_PREFIX_POLICY_KEY].is_a?(String) ||
16771684
!config[REPO_PREFIX_POLICY_KEY].match?(REPO_PREFIX_PATTERN))
16781685
issues << "invalid policy value for key: #{REPO_PREFIX_POLICY_KEY} " \
@@ -1705,7 +1712,8 @@ module AgentWorkflowSeamDoctor
17051712
if !intake_policy.key?(key)
17061713
"missing policy key: #{qualified_key}"
17071714
elsif !intake_policy[key].is_a?(String) || intake_policy[key].strip.empty? ||
1708-
unresolved_policy_value?(intake_policy[key]) || intake_policy[key].strip == "n/a"
1715+
unresolved_policy_value?(intake_policy[key], placeholder_mode: :legacy) ||
1716+
intake_policy[key].strip == "n/a"
17091717
"invalid policy value for key: #{qualified_key}"
17101718
end
17111719
end
@@ -1719,14 +1727,19 @@ module AgentWorkflowSeamDoctor
17191727
YAML.safe_load(File.read(path, encoding: "UTF-8"), aliases: false) || {}
17201728
end
17211729

1722-
def unresolved_policy_value?(value)
1730+
def unresolved_policy_value?(value, placeholder_mode:, empty_collections_unresolved: true)
17231731
case value
17241732
when String
1725-
unresolved_template_value?(value)
1733+
unresolved_template_value?(value, placeholder_mode:)
17261734
when Array
1727-
value.empty? || value.any? { |entry| unresolved_policy_value?(entry) }
1735+
(empty_collections_unresolved && value.empty?) || value.any? do |entry|
1736+
unresolved_policy_value?(entry, empty_collections_unresolved:, placeholder_mode:)
1737+
end
17281738
when Hash
1729-
value.empty? || value.any? { |key, entry| unresolved_policy_value?(key.to_s) || unresolved_policy_value?(entry) }
1739+
(empty_collections_unresolved && value.empty?) || value.any? do |key, entry|
1740+
unresolved_policy_value?(key.to_s, empty_collections_unresolved:, placeholder_mode:) ||
1741+
unresolved_policy_value?(entry, empty_collections_unresolved:, placeholder_mode:)
1742+
end
17301743
else
17311744
value.nil?
17321745
end
@@ -1758,9 +1771,12 @@ module AgentWorkflowSeamDoctor
17581771
nil
17591772
end
17601773

1761-
def unresolved_template_value?(value)
1774+
def unresolved_template_value?(value, placeholder_mode:)
17621775
stripped = value.strip
1763-
stripped.empty? || stripped.match?(SEAM_PLACEHOLDER) || stripped.match?(CI_PARITY_SEAM_PLACEHOLDER)
1776+
return true if stripped.empty?
1777+
return stripped.match?(FULL_STRING_ANGLE_PLACEHOLDER) if placeholder_mode == :full_string
1778+
1779+
stripped.match?(SEAM_PLACEHOLDER) || stripped.match?(CI_PARITY_SEAM_PLACEHOLDER)
17641780
end
17651781

17661782
def shared_markdown_paths(root, shared_roots: [])

‎bin/agent-workflow-seam-doctor-test.rb‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,48 @@ def test_unresolved_policy_value_fails
336336
end
337337
end
338338

339+
def test_required_policy_values_reject_embedded_legacy_placeholders
340+
with_repo do |root|
341+
write_valid_binstub_contract(root)
342+
write_policy(
343+
root,
344+
POLICY.merge(
345+
"base_branch" => "Use <base branch>",
346+
"ci_parity_environment" => "Run in <runner image>"
347+
)
348+
)
349+
write_skill(root, "No commands here.\n")
350+
351+
out, status = run_doctor(root)
352+
353+
refute status.success?
354+
assert_includes out, "unresolved policy value for key: base_branch"
355+
assert_includes out, "unresolved policy value for key: ci_parity_environment"
356+
end
357+
end
358+
359+
def test_optional_policy_values_recursively_reject_embedded_legacy_placeholders
360+
with_repo do |root|
361+
write_valid_binstub_contract(root)
362+
write_policy(
363+
root,
364+
POLICY.merge(
365+
"custom_scalar" => "Use <base branch>",
366+
"custom_array" => ["Run in <runner image>"],
367+
"custom_mapping" => { "nested" => "Use <main branch>" }
368+
)
369+
)
370+
write_skill(root, "No commands here.\n")
371+
372+
out, status = run_doctor(root)
373+
374+
refute status.success?
375+
%w[custom_scalar custom_array custom_mapping].each do |key|
376+
assert_includes out, "unresolved policy value for key: #{key}"
377+
end
378+
end
379+
end
380+
339381
def test_optional_repo_prefix_accepts_valid_value_and_remains_optional
340382
with_repo do |root|
341383
write_valid_binstub_contract(root)
@@ -413,6 +455,161 @@ def test_optional_autonomous_merge_policy_uses_the_shared_closed_schema
413455
end
414456
end
415457

458+
def test_optional_autonomous_merge_policy_accepts_an_exact_empty_mapping_seed
459+
with_repo do |root|
460+
write_valid_binstub_contract(root)
461+
write_policy(root, POLICY.merge("autonomous_merge" => {}))
462+
write_skill(root, "No commands here.\n")
463+
464+
policy_text = File.read(File.join(root, ".agents/agent-workflow.yml"), encoding: "UTF-8")
465+
out, status = run_doctor(root)
466+
467+
assert_includes policy_text, "autonomous_merge: {}\n"
468+
assert status.success?, out
469+
end
470+
end
471+
472+
def test_optional_autonomous_merge_policy_accepts_runtime_valid_empty_arrays
473+
with_repo do |root|
474+
write_valid_binstub_contract(root)
475+
write_policy(
476+
root,
477+
POLICY.merge(
478+
"autonomous_merge" => {
479+
"human_review_paths" => [],
480+
"policy_paths" => [],
481+
"generated_paths" => []
482+
}
483+
)
484+
)
485+
write_skill(root, "No commands here.\n")
486+
487+
out, status = run_doctor(root)
488+
489+
assert status.success?, out
490+
end
491+
end
492+
493+
def test_optional_autonomous_merge_policy_rejects_nested_unresolved_placeholders
494+
variants = {
495+
"legacy direct list scalar" => {
496+
"policy_paths" => ["<base branch>"]
497+
},
498+
"legacy nested mapping list scalar" => {
499+
"safe_path_groups" => {
500+
"documentation" => {
501+
"include" => ["docs/**"],
502+
"exclude" => ["<base branch>"]
503+
}
504+
}
505+
},
506+
"threshold relaxation rationale" => {
507+
"thresholds" => { "max_changed_files" => 30 },
508+
"threshold_relaxation" => {
509+
"rationale" => "<nonempty rationale covering all relaxed thresholds>"
510+
}
511+
},
512+
"policy path" => {
513+
"policy_paths" => ["<repo-owned glob>"]
514+
},
515+
"generated path" => {
516+
"generated_paths" => ["<repo-owned generated glob>"]
517+
},
518+
"human-review pattern" => {
519+
"human_review_paths" => [
520+
{
521+
"id" => "repo-owned-risk",
522+
"pattern" => "<repo-owned glob>",
523+
"reason" => "hot-path"
524+
}
525+
]
526+
},
527+
"human-review other detail" => {
528+
"human_review_paths" => [
529+
{
530+
"id" => "repo-owned-risk",
531+
"pattern" => "app/**",
532+
"reason" => "other",
533+
"detail" => "<nonempty repo-owned reason>"
534+
}
535+
]
536+
},
537+
"safe-path include" => {
538+
"safe_path_groups" => {
539+
"documentation" => {
540+
"include" => ["<repo-owned glob>"],
541+
"exclude" => []
542+
}
543+
}
544+
},
545+
"safe-path exclude" => {
546+
"safe_path_groups" => {
547+
"documentation" => {
548+
"include" => ["docs/**"],
549+
"exclude" => ["<repo-owned glob>"]
550+
}
551+
}
552+
}
553+
}
554+
results = variants.transform_values do |autonomous_merge|
555+
with_repo do |root|
556+
write_valid_binstub_contract(root)
557+
write_policy(root, POLICY.merge("autonomous_merge" => autonomous_merge))
558+
write_skill(root, "No commands here.\n")
559+
560+
run_doctor(root)
561+
end
562+
end
563+
failures = results.filter_map do |label, (out, status)|
564+
next unless status.success? || !out.include?("unresolved policy value for key: autonomous_merge")
565+
566+
"#{label}: status=#{status.exitstatus}, output=#{out.inspect}"
567+
end
568+
569+
assert_empty failures, failures.join("\n")
570+
end
571+
572+
def test_optional_autonomous_merge_policy_allows_angle_bracket_text_inside_ordinary_prose
573+
rationales = [
574+
"Document <nonempty rationale covering all relaxed thresholds> after calibration.",
575+
"Document <base branch> as historical context after calibration."
576+
]
577+
rationales.each do |rationale|
578+
with_repo do |root|
579+
write_valid_binstub_contract(root)
580+
write_policy(
581+
root,
582+
POLICY.merge(
583+
"autonomous_merge" => {
584+
"thresholds" => { "max_changed_files" => 30 },
585+
"threshold_relaxation" => { "rationale" => rationale }
586+
}
587+
)
588+
)
589+
write_skill(root, "No commands here.\n")
590+
591+
out, status = run_doctor(root)
592+
593+
assert status.success?, out
594+
end
595+
end
596+
end
597+
598+
def test_empty_collections_outside_autonomous_merge_remain_unresolved
599+
[[], {}].each do |empty_collection|
600+
with_repo do |root|
601+
write_valid_binstub_contract(root)
602+
write_policy(root, POLICY.merge("custom_runtime_paths" => empty_collection))
603+
write_skill(root, "No commands here.\n")
604+
605+
out, status = run_doctor(root)
606+
607+
refute status.success?, empty_collection.class.name
608+
assert_includes out, "unresolved policy value for key: custom_runtime_paths", empty_collection.class.name
609+
end
610+
end
611+
end
612+
416613
def test_invalid_autonomous_merge_policy_is_reported_by_the_seam_doctor
417614
with_repo do |root|
418615
write_valid_binstub_contract(root)

‎skills/pr-batch/SKILL.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,10 @@ Ask only for missing data. If the user already supplied an exact value, use it.
207207
10. **Lane split**: exact per-machine list, odd/even, labels, area, owner, or another explicit partition.
208208
11. **Permissions**: confirm the current session can run without blocking worker approval prompts.
209209
12. **Question handling**: labels or comments to use for blocking questions, plus where non-blocking decisions should be recorded.
210-
13. **Completion states**: `merged`, `ready-gates-clean`, `ready-no-merge-authority`, `waiting-on-checks-or-review`, `external-gate-failing`, `blocked-user-input`, or `no-pr-evidence`.
210+
13. **Completion states**: `merged`, `ready-gates-clean`, `ready-no-merge-authority`,
211+
`ready-human-review-required`, `autonomous-merge-evidence-unknown`,
212+
`waiting-on-checks-or-review`, `external-gate-failing`, `blocked-user-input`,
213+
or `no-pr-evidence`.
211214

212215
## Canonical Readiness Vocabulary
213216

0 commit comments

Comments
 (0)