diff --git a/bin/hive-eval b/bin/hive-eval index e77c243a55..cba090613d 100755 --- a/bin/hive-eval +++ b/bin/hive-eval @@ -253,6 +253,10 @@ if path end if inherited_rake_dry_run?(ENV["RAKEOPT"]) + # A rejected inherited RAKEOPT must not leave a stale default report behind, + # matching the other pre-run guards that exit before the shared removal point. + # Custom report paths are still only cleared after full validation, below. + remove_report!(report_path) if report_path == default_report warn "hive-eval: inherited RAKEOPT may not enable rake dry-run" exit 1 end diff --git a/components/agent-cli-runtime/lib/agent_cli_runtime/opencode/probe.rb b/components/agent-cli-runtime/lib/agent_cli_runtime/opencode/probe.rb index f32d19a952..507a5ffaf6 100644 --- a/components/agent-cli-runtime/lib/agent_cli_runtime/opencode/probe.rb +++ b/components/agent-cli-runtime/lib/agent_cli_runtime/opencode/probe.rb @@ -173,16 +173,26 @@ def child_environment(profile, request, env:) selected[key] = value.to_s end end - %w[ - BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT - ].each { |key| selected[key] = nil } profile.env_bin_override_keys.each do |key| selected[key] = env[key].to_s unless env[key].to_s.empty? end request.credential_environment_keys.each do |key| selected[key] = env[key].to_s unless env[key].to_s.empty? end - selected.merge(request.environment).freeze + # The overlay is the only caller-supplied channel into the child + # environment. Filtering here keeps the denylist cleanup final even if + # a request is built with keys outside the validated overlay contract. + request.environment.each do |key, value| + next unless OPENCODE_OVERLAY_ENVIRONMENT_KEYS.include?(key) + + selected[key] = value.to_s + end + # The denylist scrub must stay last: every caller-controlled channel + # above (credential keys, executable overrides, the overlay) copies + # selected values into the child environment, so no selection may + # reintroduce a scrubbed runtime injection key. + SCRUBBED_RUNTIME_ENVIRONMENT_KEYS.each { |key| selected[key] = nil } + selected.freeze end private_class_method :child_environment diff --git a/components/agent-cli-runtime/lib/agent_cli_runtime/values.rb b/components/agent-cli-runtime/lib/agent_cli_runtime/values.rb index f7e3db41d1..bb46a67cb9 100644 --- a/components/agent-cli-runtime/lib/agent_cli_runtime/values.rb +++ b/components/agent-cli-runtime/lib/agent_cli_runtime/values.rb @@ -6,6 +6,14 @@ module AgentCliRuntime OPENCODE_DISABLE_AUTOUPDATE OPENCODE_PURE ].freeze + # Environment keys that Ruby, Bundler, and RubyGems honor at startup to + # inject load paths. The OpenCode probe scrubs them from every child + # environment, so request constructors must refuse to select them back as + # credential keys. + SCRUBBED_RUNTIME_ENVIRONMENT_KEYS = %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].freeze + module Immutable module_function @@ -301,6 +309,11 @@ def initialize(request:, working_directory:, invocation_root:, raise ArgumentError, "credential environment keys cannot override the OpenCode overlay: #{reserved.join(', ')}" end + scrubbed = keys & SCRUBBED_RUNTIME_ENVIRONMENT_KEYS + unless scrubbed.empty? + raise ArgumentError, + "credential environment keys cannot restore scrubbed runtime injection keys: #{scrubbed.join(', ')}" + end normalized_bash_patterns = Immutable.strings(bash_patterns) if normalized_bash_patterns.any?(&:empty?) raise ArgumentError, "OpenCode bash patterns must be non-empty" @@ -339,13 +352,24 @@ def initialize(profile:, route:, variant: nil, environment: {}, configured_variants: nil, executable: nil) parsed_route = route.is_a?(Route) ? route : Route.parse(route) + normalized_environment = Immutable.hash(environment) + unallowed = normalized_environment.keys - OPENCODE_OVERLAY_ENVIRONMENT_KEYS + unless unallowed.empty? + raise ArgumentError, + "ProbeRequest environment keys must be OpenCode overlay keys: #{unallowed.join(', ')}" + end + normalized_credential_keys = Immutable.strings(credential_environment_keys) + scrubbed = normalized_credential_keys & SCRUBBED_RUNTIME_ENVIRONMENT_KEYS + unless scrubbed.empty? + raise ArgumentError, + "ProbeRequest credential environment keys cannot restore scrubbed runtime injection keys: #{scrubbed.join(', ')}" + end super( profile: profile, route: parsed_route, variant: variant.nil? ? nil : Immutable.string(variant), - environment: Immutable.hash(environment), - credential_environment_keys: - Immutable.strings(credential_environment_keys), + environment: normalized_environment, + credential_environment_keys: normalized_credential_keys, credential_file_staged: credential_file_staged == true, configured_variants: configured_variants.nil? ? nil : Immutable.strings(configured_variants), diff --git a/components/agent-cli-runtime/test/opencode_preparation_test.rb b/components/agent-cli-runtime/test/opencode_preparation_test.rb index 2d3fc25e1b..61fcb9b28f 100644 --- a/components/agent-cli-runtime/test/opencode_preparation_test.rb +++ b/components/agent-cli-runtime/test/opencode_preparation_test.rb @@ -615,6 +615,54 @@ def test_probe_environment_explicitly_unsets_ambient_unselected_values ENV.delete("UNSELECTED_PROBE_SECRET") end + def test_probe_request_rejects_environment_keys_outside_the_overlay_contract + profile = AgentCliRuntime::Profiles.fetch(:opencode) + %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].each do |key| + error = assert_raises(ArgumentError) do + AgentCliRuntime::ProbeRequest.new( + profile:, route: "anthropic/claude-sonnet-4-5", + environment: { key => "restored-injection" } + ) + end + assert_match(/must be OpenCode overlay keys/, error.message) + assert_includes error.message, key + end + end + + def test_probe_child_environment_keeps_scrubbed_runtime_keys_nil_across_the_overlay_merge + profile = AgentCliRuntime::Profiles.fetch(:opencode) + request = AgentCliRuntime::ProbeRequest.new( + profile:, route: "anthropic/claude-sonnet-4-5", + environment: { + "XDG_CONFIG_HOME" => "/fixture/config", + "XDG_DATA_HOME" => "/fixture/data", + "TMPDIR" => "/fixture/tmp" + } + ) + + child = AgentCliRuntime::OpenCode::Probe.send( + :child_environment, profile, request, + env: { + "PATH" => ENV.fetch("PATH"), + "HOME" => "/fixture/home", + "RUBYOPT" => "-r/ambient/missing", + "GEM_HOME" => "/fixture/gems" + } + ) + + %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].each do |key| + assert_nil child[key], "expected #{key} to stay scrubbed in the child environment" + end + assert_equal "/fixture/config", child.fetch("XDG_CONFIG_HOME") + assert_equal "/fixture/data", child.fetch("XDG_DATA_HOME") + assert_equal "/fixture/tmp", child.fetch("TMPDIR") + assert_equal "/fixture/home", child.fetch("HOME") + end + def test_credential_environment_keys_cannot_override_overlay_controls error = assert_raises(ArgumentError) do AgentCliRuntime::OpenCodePreparationRequest.new( @@ -627,6 +675,69 @@ def test_credential_environment_keys_cannot_override_overlay_controls assert_match(/cannot override the OpenCode overlay/, error.message) end + def test_probe_request_rejects_scrubbed_runtime_keys_as_credential_keys + profile = AgentCliRuntime::Profiles.fetch(:opencode) + %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].each do |key| + error = assert_raises(ArgumentError) do + AgentCliRuntime::ProbeRequest.new( + profile:, route: "anthropic/claude-sonnet-4-5", + credential_environment_keys: [ key ] + ) + end + assert_match(/cannot restore scrubbed runtime injection keys/, error.message) + assert_includes error.message, key + end + end + + def test_preparation_request_rejects_scrubbed_runtime_keys_as_credential_keys + %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].each do |key| + error = assert_raises(ArgumentError) do + AgentCliRuntime::OpenCodePreparationRequest.new( + request: AgentCliRuntime::Request.new(profile: :opencode, prompt: "test"), + working_directory: Dir.pwd, + invocation_root: File.join(Dir.tmpdir, "unused-opencode-root"), + credential_environment_keys: [ key ] + ) + end + assert_match(/cannot restore scrubbed runtime injection keys/, error.message) + assert_includes error.message, key + end + end + + def test_probe_child_environment_keeps_scrubbed_runtime_keys_nil_across_credential_selection + profile = AgentCliRuntime::Profiles.fetch(:opencode) + request = AgentCliRuntime::ProbeRequest.new( + profile:, route: "anthropic/claude-sonnet-4-5", + environment: { + "XDG_CONFIG_HOME" => "/fixture/config" + }, + credential_environment_keys: [ "ANTHROPIC_API_KEY" ] + ) + + child = AgentCliRuntime::OpenCode::Probe.send( + :child_environment, profile, request, + env: { + "PATH" => ENV.fetch("PATH"), + "RUBYOPT" => "-r/ambient/missing", + "GEM_HOME" => "/ambient/gems", + "ANTHROPIC_API_KEY" => "secret-canary" + } + ) + + %w[ + BUNDLE_BIN_PATH BUNDLE_GEMFILE GEM_HOME GEM_PATH RUBYLIB RUBYOPT + ].each do |key| + assert_nil child[key], + "expected #{key} to stay scrubbed despite ambient values" + end + assert_equal "/fixture/config", child.fetch("XDG_CONFIG_HOME") + assert_equal "secret-canary", child.fetch("ANTHROPIC_API_KEY") + end + def test_route_probe_is_fail_soft_when_the_binary_is_missing request = AgentCliRuntime::ProbeRequest.new( profile: :opencode, diff --git a/lib/hive/bot/brainstorm_answer_writer.rb b/lib/hive/bot/brainstorm_answer_writer.rb index 75cecf8226..5991b5c456 100644 --- a/lib/hive/bot/brainstorm_answer_writer.rb +++ b/lib/hive/bot/brainstorm_answer_writer.rb @@ -27,6 +27,17 @@ module BrainstormAnswerWriter ROUND_RE = Hive::Bot::BrainstormParser::ROUND_RE MARKER_RE = Hive::Bot::BrainstormParser::MARKER_RE + # The physical line array all writer surgery operates on: the + # parser's `document_lines` (newline-normalized, chomped — exactly + # the array the parser's `question_line_index` / `block_end_index` / + # `answer_line_index` fields index into), re-terminated with `\n` so + # a plain join reproduces the document. Parser location fields and + # writer line surgery therefore share one definition of "a line". + def write_document_lines(content) + Hive::BrainstormParser.document_lines(content).map { |line| "#{line}\n" } + end + private_class_method :write_document_lines + LOCK_RETRY_DEADLINE_SEC = 5 LOCK_RETRY_SLEEP_SEC = 0.05 MARKER_LOCK_TIMEOUT_SEC = 5 @@ -109,25 +120,30 @@ def write_at_ordinal_under_lock!(brainstorm_path:, ordinal:, answer_text:) Hive::Markers.with_markers_lock( brainstorm_path, create: false, timeout: MARKER_LOCK_TIMEOUT_SEC ) do - content = normalize_lone_cr(File.read(brainstorm_path, encoding: "UTF-8").scrub) + # Line surgery must happen on the SAME newline-normalized line + # array the parser indexed its location fields against + # (`document_lines`), not on `content.lines`: a lone-\r file + # yields more parser lines than raw lines, so index math across + # the two would misattribute. See `write_document_lines`. + content = File.read(brainstorm_path, encoding: "UTF-8").scrub + lines = write_document_lines(content) parsed = Hive::Bot::BrainstormParser.parse_text(content) target = parsed[ordinal - 1] next :question_not_found unless target next :already_answered if target.answered? - lines = content.lines - question_line_index = question_line_index_for_ordinal(lines, ordinal) + question_line_index = target.question_line_index next :question_not_found unless question_line_index - slot = find_empty_answer_slot_after(lines, question_line_index) + slot = find_empty_answer_slot(lines, target) new_lines = if slot - fill_answer_slot(lines, slot, answer_text, content) + fill_answer_slot(lines, slot, answer_text) else insert_answer_slot_after( - lines, question_line_index, target.n, answer_text, content + lines, question_line_index, target.block_end_index, target.n, answer_text ) end - Hive::Markers.write_atomic(brainstorm_path, new_lines.join) + Hive::Markers.write_atomic(brainstorm_path, join_document(new_lines, content)) :written end end @@ -143,7 +159,13 @@ def write_at_ordinal_under_lock!(brainstorm_path:, ordinal:, answer_text:) def try_append(task_folder, brainstorm_path, question_n, answer_text) result = Hive::Lock.with_task_lock(task_folder, "bot" => "brainstorm_answer") do Hive::Markers.with_markers_lock(brainstorm_path, create: false) do + # Line surgery must happen on the SAME newline-normalized line + # array the parser indexed its location fields against + # (`document_lines`), not on `content.lines`: a lone-\r file + # yields more parser lines than raw lines, so index math across + # the two would misattribute. See `write_document_lines`. content = File.exist?(brainstorm_path) ? File.read(brainstorm_path, encoding: "UTF-8").scrub : "" + lines = write_document_lines(content) parsed = Hive::Bot::BrainstormParser.parse_text(content) if !parsed.any? { |question| question.n == question_n } next :question_not_found @@ -152,10 +174,10 @@ def try_append(task_folder, brainstorm_path, question_n, answer_text) next :already_answered end - lines = content.lines - slot = find_empty_answer_slot(lines, question_n, parsed) + target = parsed.find { |question| question.n == question_n && question.answer.nil? } + slot = find_empty_answer_slot(lines, target) if slot - new_lines = fill_answer_slot(lines, slot, answer_text, content) + new_lines = fill_answer_slot(lines, slot, answer_text) else # Q{n} is unanswered (the earlier guards verified Q{n} is # present and its answer is nil) but has NO `### A{n}.` header @@ -165,11 +187,11 @@ def try_append(task_folder, brainstorm_path, question_n, answer_text) # answer and, with the daemon's answers-pending gate, held the # task indefinitely — issue #269), CREATE the slot at the end # of the Q-block and write the answer into it. - new_lines = insert_answer_slot(lines, question_n, parsed, answer_text, content) + new_lines = insert_answer_slot(lines, target, answer_text) next :answer_slot_missing unless new_lines end - Hive::Markers.write_atomic(brainstorm_path, new_lines.join) + Hive::Markers.write_atomic(brainstorm_path, join_document(new_lines, content)) :written end end @@ -185,14 +207,26 @@ def try_append(task_folder, brainstorm_path, question_n, answer_text) end private_class_method :try_append + # Rebuild a brainstorm document from its lines. Every line in the + # array is newline-terminated, so a plain join is enough; the only + # decision made here is the line-ending style, which follows the + # pre-write file (CRLF stays CRLF, LF stays LF) instead of being + # decided per-inserted-line by a separate `newline_for` rule that + # could drift from the document around it. + def join_document(lines, content) + text = lines.join + content.include?("\r\n") ? text.gsub("\n", "\r\n") : text + end + private_class_method :join_document + # Write `answer_text` into an existing empty `### A` slot. - def fill_answer_slot(lines, slot, answer_text, content) - newline = newline_for(content) - lines[slot[:answer_line_index]] = - "#{Hive::Bot::BrainstormParser.encoded_answer_header(slot.fetch(:question_n))}#{newline}" + def fill_answer_slot(lines, slot, answer_text) + answer_line = slot.fetch(:answer_line_index) + block_end = slot.fetch(:block_end_index) - answer_lines = answer_body(answer_text, newline).lines - lines[0..slot[:answer_line_index]] + answer_lines + lines[slot[:body_end_index]..].to_a + lines[answer_line] = "#{Hive::Bot::BrainstormParser.encoded_answer_header(slot.fetch(:question_n))}\n" + + lines[0..answer_line] + answer_body(answer_text).lines + lines[block_end..].to_a end private_class_method :fill_answer_slot @@ -200,138 +234,75 @@ def fill_answer_slot(lines, slot, answer_text, content) # at the end of the Q-block (just before the next Q / Round / marker # boundary, or EOF), and write the answer into it. Uses the parser's # canonical `answer_header` so the format stays in lockstep. Returns - # the new lines, or nil when the Q{n} line can't be located (the - # earlier guard makes this unreachable in practice; nil routes + # the new lines, or nil when the parser could not locate the target + # question's block span (unreachable in practice; nil routes # `try_append` back to the `:answer_slot_missing` fallback). - def insert_answer_slot(lines, question_n, parsed, answer_text, content) - q_idx = target_question_line_index(lines, parsed, question_n) - return nil unless q_idx + def insert_answer_slot(lines, target, answer_text) + return nil unless target&.question_line_index - insert_answer_slot_after(lines, q_idx, question_n, answer_text, content) + insert_answer_slot_after( + lines, target.question_line_index, target.block_end_index, target.n, answer_text + ) end private_class_method :insert_answer_slot - def insert_answer_slot_after(lines, q_idx, question_n, answer_text, content) - newline = newline_for(content) - insert_idx = q_idx + 1 - insert_idx += 1 while insert_idx < lines.length && !block_boundary?(lines[insert_idx]) - # Ensure the preceding line is newline-terminated so the new - # header doesn't get glued onto the question's last body line. - if insert_idx.positive? && !lines[insert_idx - 1].to_s.end_with?(newline) - lines[insert_idx - 1] = "#{lines[insert_idx - 1]}#{newline}" - end + def insert_answer_slot_after(lines, q_idx, block_end_idx, question_n, answer_text) + # `block_end_idx` is the parser-computed first boundary line of the + # Q-block (or EOF), so the slot lands exactly at the block's end; + # every line in `lines` is newline-terminated, so no + # newline-termination repair is needed before splicing. + insert_idx = block_end_idx || q_idx + 1 - slot_lines = [ "#{Hive::Bot::BrainstormParser.encoded_answer_header(question_n)}#{newline}" ] + - answer_body(answer_text, newline).lines + slot_lines = [ "#{Hive::Bot::BrainstormParser.encoded_answer_header(question_n)}\n" ] + + answer_body(answer_text).lines lines[0...insert_idx] + slot_lines + lines[insert_idx..].to_a end private_class_method :insert_answer_slot_after - # Locate the empty A-section to fill for question_n. Q-context- - # aware: walks forward from the Q{n} header that the parser - # identified as the target (first unanswered Q with that number - # in document order) and returns the first empty A-section before - # the next block boundary (Q / Round / marker). + # Locate the empty A-section to fill for the parser-identified target + # question. Q-context-aware BY CONSTRUCTION: the target comes from + # the parser (first unanswered Q with that number in document order) + # together with its block span, so there is no writer-side re-scan + # that could disagree with the parser about which Q header or block + # boundary applies. # # Tolerates off-by-one A-numbers (agents occasionally emit # `### A2.` after `### Q1.`) without misattributing: the slot is # selected by POSITION within the Q-block, not by A-number. # - # Previously this was a two-step scan: strict by-number first, - # then by-position fallback. The strict scan ignored Q-context - # and could cross round boundaries — a brainstorm.md with empty - # `### A1.` in Round 1 (still unanswered) and Round 2's `### A1.` - # would route operator's Round-2 answer into Round-1's slot. The - # combined Q-context-aware function below resolves that by - # always anchoring the scan to the parser-identified target Q's - # line, then walking forward. - def find_empty_answer_slot(lines, question_n, parsed) - q_idx = target_question_line_index(lines, parsed, question_n) - return nil unless q_idx - - find_empty_answer_slot_after(lines, q_idx) - end - private_class_method :find_empty_answer_slot - - def find_empty_answer_slot_after(lines, q_idx) - scan = q_idx + 1 - while scan < lines.length - stripped = lines[scan].chomp - break if stripped =~ QUESTION_RE || stripped =~ ROUND_RE || stripped =~ MARKER_RE - return empty_slot_starting_at(lines, scan) if ANSWER_RE.match?(stripped) - - scan += 1 - end - nil - end - private_class_method :find_empty_answer_slot_after - - def question_line_index_for_ordinal(lines, ordinal) - seen = 0 - lines.each_with_index do |line, idx| - next unless QUESTION_RE.match?(line.chomp) - - seen += 1 - return idx if seen == ordinal - end - nil - end - private_class_method :question_line_index_for_ordinal - - # Map the parser's view of "first unanswered Q with this number" - # back to a line index in the raw file. Multiple Q{n} can exist - # (e.g. each new Round restarts numbering); we want the one the - # parser believes is still awaiting an answer. - def target_question_line_index(lines, parsed, question_n) - matching = parsed.select { |q| q.n == question_n } - target_position = matching.find_index { |q| q.answer.nil? } - return nil unless target_position - - seen = 0 - lines.each_with_index do |line, idx| - match = QUESTION_RE.match(line.chomp) - next unless match && match[1].to_i == question_n - return idx if seen == target_position - - seen += 1 - end - nil - end - private_class_method :target_question_line_index - - # Given an index pointing at an `### A{x}.` line, return the slot - # envelope iff the body (until the next block boundary) is empty. - def empty_slot_starting_at(lines, idx) - body_end = idx + 1 - body = [] - while body_end < lines.length && !block_boundary?(lines[body_end]) - body << lines[body_end] - body_end += 1 - end + # Previously the target line index was re-derived here by matching + # raw lines against the parsed question list (`target_question_line_index`) + # and the block boundary was re-derived by regex-scanning forward + # (`block_boundary?`). The strict scan ignored Q-context and could + # cross round boundaries — a brainstorm.md with empty `### A1.` in + # Round 1 (still unanswered) and Round 2's `### A1.` would route the + # operator's Round-2 answer into Round-1's slot. Anchoring on the + # parser's location fields resolves that by construction. + def find_empty_answer_slot(lines, target) + return nil unless target&.question_line_index && target&.block_end_index + + a_idx = target.answer_line_index + return nil unless a_idx && a_idx > target.question_line_index && a_idx < target.block_end_index + + body = lines[(a_idx + 1)...target.block_end_index].to_a return nil unless body.join.strip.empty? - match = ANSWER_RE.match(lines[idx].chomp) + match = ANSWER_RE.match(lines[a_idx].to_s.chomp) { - answer_line_index: idx, - body_end_index: body_end, + answer_line_index: a_idx, + block_end_index: target.block_end_index, question_n: match[1].to_i } end - private_class_method :empty_slot_starting_at - - def block_boundary?(line) - stripped = line.chomp - QUESTION_RE.match?(stripped) || ROUND_RE.match?(stripped) || MARKER_RE.match?(stripped) - end - private_class_method :block_boundary? + private_class_method :find_empty_answer_slot - def answer_body(answer_text, newline) + def answer_body(answer_text) text = answer_text.to_s.gsub("\r\n", "\n").gsub("\r", "\n").rstrip - return newline if text.empty? + return "\n" if text.empty? text.lines(chomp: true).map do |line| encoded = escape_answer_line?(line) ? encode_answer_line(line) : line - "#{encoded}#{newline}" + "#{encoded}\n" end.join end private_class_method :answer_body @@ -352,16 +323,6 @@ def structural_heading?(line) ROUND_RE.match?(line) || QUESTION_RE.match?(line) || ANSWER_RE.match?(line) end private_class_method :structural_heading? - - def newline_for(content) - content.include?("\r\n") ? "\r\n" : "\n" - end - private_class_method :newline_for - - def normalize_lone_cr(content) - content.to_s.gsub(/\r(?!\n)/, "\n") - end - private_class_method :normalize_lone_cr end end end diff --git a/lib/hive/bot/handlers/callback_handlers.rb b/lib/hive/bot/handlers/callback_handlers.rb index a0a31c4f86..5d0b45cca1 100644 --- a/lib/hive/bot/handlers/callback_handlers.rb +++ b/lib/hive/bot/handlers/callback_handlers.rb @@ -403,7 +403,10 @@ def collect_files_for_draft(project, token) return @result_class.new(action: :reply, text: "That idea picker expired. Send /idea again.") unless draft @idea_draft_store.set_project(chat_id: draft.chat_id, project: project) - if draft.origin == :voice && draft.attachments.empty? + # The immediate-commit-vs-file-collection decision belongs to the + # store: only a voice draft with a confirmed transcript and no staged + # fallback audio commits straight away. + if @idea_draft_store.transcript_only_voice_draft?(token: token) @set_last_project.call(project) return @result_class.new(action: :commit_idea, project: project, attachment: { chat_id: draft.chat_id }, clear_keyboard: true) diff --git a/lib/hive/bot/idea_draft_store.rb b/lib/hive/bot/idea_draft_store.rb index 642a1ca71e..ee25371406 100644 --- a/lib/hive/bot/idea_draft_store.rb +++ b/lib/hive/bot/idea_draft_store.rb @@ -19,6 +19,12 @@ class IdeaDraftStore # than by the type. PHASES enumerates every legal phase; ORIGINS the # legal non-nil origins (nil = a plain typed/media draft). A typo'd # symbol raises at the transition rather than silently mis-routing. + # + # Because callers receive this mutable representation, compound state + # interpretations (phase + origin + attachments) and the commit-time + # view must be decided HERE, not re-derived from raw fields by the + # router, callback handlers, or supervisor. The decision predicates and + # commit_snapshot below are that single authority. PHASES = %i[awaiting_text awaiting_project awaiting_transcript_confirm collecting_files].freeze ORIGINS = %i[voice].freeze @@ -66,6 +72,111 @@ def find_by_token(token) @drafts.each_value.find { |draft| draft.token == token && !expired?(draft) } end + # ---- State decisions ---- + # Compound interpretations of a draft's phase/origin/attachments that + # routing and callbacks depend on. Consumers must ask these instead of + # reading raw Draft fields, so the phase/origin contract keeps a single + # authority alongside the transition methods above. + + # A live draft sitting on the voice transcript Confirm/Discard keyboard: + # the router routes follow-up voice/text to the confirm flow only here. + def awaiting_transcript_confirmation?(chat_id:) + draft = get(chat_id: chat_id) + !draft.nil? && draft.phase == :awaiting_transcript_confirm && draft.origin == :voice + end + + # A live draft that is NOT voice-origin: a voice note arriving now must + # be short-circuited (VOICE_DURING_DRAFT_MESSAGE) instead of merged in. + def non_voice_draft?(chat_id:) + draft = get(chat_id: chat_id) + !draft.nil? && draft.origin != :voice + end + + # A live voice-origin draft: the only kind a voice note may reuse or + # clear without destroying an operator's typed/media work. + def voice_draft?(chat_id:) + draft = get(chat_id: chat_id) + !draft.nil? && draft.origin == :voice + end + + # A live draft still waiting for its idea text, so the next plain text + # message continues idea capture instead of opening a new one. + def awaiting_text_draft?(chat_id:) + draft = get(chat_id: chat_id) + !draft.nil? && draft.phase == :awaiting_text + end + + # A voice draft reached through its callback token whose transcript is + # confirmed and which has no staged fallback audio: it can commit + # immediately instead of entering the file-collection flow. + def transcript_only_voice_draft?(token:) + draft = find_by_token(token) + !draft.nil? && draft.origin == :voice && draft.attachments.empty? + end + + # The voice-draft reuse decision for an incoming voice note: reuse a + # live voice-origin draft, start a fresh one when no draft exists, and + # return nil when a non-voice (text/media) draft is open so the caller + # never clobbers the operator's in-progress work. Returns the draft to + # act on, or nil. + def ensure_voice_draft(chat_id:, token:) + existing = get(chat_id: chat_id) + return existing if existing&.origin == :voice + return nil if existing + + start(chat_id: chat_id, phase: :awaiting_transcript_confirm, + token: token, origin: :voice) + end + + # Clear only a voice-origin draft. A :no_speech / :unsupported_language + # voice note must not wipe an unrelated in-progress text/media draft + # (and its staging dir) the operator was building. + def clear_voice_draft(chat_id:) + draft = get(chat_id: chat_id) + clear(chat_id: chat_id) if draft&.origin == :voice + end + + # Commit gate: nil when the draft can be committed, otherwise the reason + # it cannot (:draft_expired, :project_missing, :text_missing). Callers + # map the reason to operator copy; the decision lives here. + def commit_blocker(chat_id:) + draft = get(chat_id: chat_id) + return :draft_expired unless draft + return :project_missing if draft.project.to_s.empty? + return :text_missing if draft.text.to_s.strip.empty? + + nil + end + + # Frozen execution view for commit: project, text, and the attachment + # tuples execution needs (staging_path, dest_name, ext). Execution must + # not depend on the live mutable Draft, which transition methods keep + # mutating; this snapshot cannot drift under it. + # + # Freezing the containers alone is not enough: freezing an object does + # not deep-freeze its contents, so every String leaf is also duplicated + # and frozen here. Without that the snapshot would still alias the live + # draft's mutable String values and in-place edits (<<, replace) on the + # live draft would rewrite what was already handed to execution. + CommitSnapshot = Struct.new(:project, :text, :attachments, keyword_init: true) + + def commit_snapshot(chat_id:) + draft = get(chat_id: chat_id) + return nil unless draft + + CommitSnapshot.new( + project: frozen_string(draft.project), + text: frozen_string(draft.text), + attachments: draft.attachments.map do |attachment| + { + staging_path: frozen_string(attachment.fetch(:staging_path)), + dest_name: frozen_string(attachment.fetch(:dest_name)), + ext: frozen_string(attachment.fetch(:ext)) + }.freeze + end.freeze + ).freeze + end + def set_text(chat_id:, text:) update(chat_id: chat_id) do |draft| draft.text = text @@ -150,6 +261,13 @@ def prune! private + # Deep-freeze helper for snapshot leaves: freeze does not apply to a + # container's contents, so every String handed into a frozen snapshot + # container is duplicated and frozen individually (nil passes through). + def frozen_string(value) + value.nil? ? nil : value.dup.freeze + end + def assign_phase!(draft, phase) validate_phase!(phase) # Enforce the phase/origin coupling at the transition too, not only at diff --git a/lib/hive/bot/router.rb b/lib/hive/bot/router.rb index 69cfe0860b..ad48b398cc 100644 --- a/lib/hive/bot/router.rb +++ b/lib/hive/bot/router.rb @@ -172,18 +172,17 @@ def classify(update) return :free_text_answer end - draft = @idea_draft_store.get(chat_id: update.chat_id) - if draft&.phase == :awaiting_transcript_confirm && draft.origin == :voice + if @idea_draft_store.awaiting_transcript_confirmation?(chat_id: update.chat_id) return :idea_voice if update.respond_to?(:voice?) && update.voice? return :idea_voice_edit_text if update.respond_to?(:text?) && update.text? end if update.respond_to?(:voice?) && update.voice? - return :idea_voice_during_draft if draft && draft.origin != :voice + return :idea_voice_during_draft if @idea_draft_store.non_voice_draft?(chat_id: update.chat_id) return :idea_voice end return :idea_media if update.respond_to?(:media?) && update.media? - return :idea_text_capture if draft&.phase == :awaiting_text + return :idea_text_capture if @idea_draft_store.awaiting_text_draft?(chat_id: update.chat_id) # Bare non-slash text now defaults to idea capture. Text-less updates # with no media/voice (stickers, locations, contacts, video) reach diff --git a/lib/hive/bot/supervisor.rb b/lib/hive/bot/supervisor.rb index f22cc504d5..0fe8b06e2b 100644 --- a/lib/hive/bot/supervisor.rb +++ b/lib/hive/bot/supervisor.rb @@ -575,7 +575,7 @@ def execute_transcribe_voice(result, update) payload = result.attachment || {} chat_id = payload.fetch(:chat_id, update.chat_id) - if !answer_voice && non_voice_draft?(chat_id: chat_id) + if !answer_voice && @idea_draft_store.non_voice_draft?(chat_id: chat_id) return safe_send_message(chat_id: update.chat_id, text: Hive::Bot::IdeaDraftStore::VOICE_DURING_DRAFT_MESSAGE) end @@ -731,11 +731,6 @@ def remote_file_too_large?(file_size) size.positive? && size > idea_attachment_max_bytes end - def non_voice_draft?(chat_id:) - draft = @idea_draft_store.get(chat_id: chat_id) - draft && draft.origin != :voice - end - def idea_attachment_max_bytes @config.fetch("idea_attachment_max_bytes", 20 * 1024 * 1024).to_i end @@ -744,25 +739,15 @@ def idea_attachment_max_mb (idea_attachment_max_bytes.to_f / (1024 * 1024)).round end - # Reuse an existing draft ONLY when it is voice-origin. A voice note - # arriving mid-text/-media draft must not overwrite the operator's typed - # text or merge into their media draft, so for any non-voice draft we - # start a fresh voice draft instead of clobbering that work in place. def ensure_voice_draft(chat_id:) - existing = @idea_draft_store.get(chat_id: chat_id) - return existing if existing&.origin == :voice - return nil if existing - - @idea_draft_store.start(chat_id: chat_id, phase: :awaiting_transcript_confirm, - token: SecureRandom.hex(4), origin: :voice) + # The reuse-vs-start decision (voice-origin only, never clobber a + # text/media draft) belongs to the store; this delegator keeps the + # supervisor call sites and test seams unchanged. + @idea_draft_store.ensure_voice_draft(chat_id: chat_id, token: SecureRandom.hex(4)) end - # Clear only a voice-origin draft. A :no_speech / :unsupported_language - # voice note must not wipe an unrelated in-progress text/media draft (and - # its staging dir) the operator was building. def clear_voice_draft(chat_id:) - draft = @idea_draft_store.get(chat_id: chat_id) - @idea_draft_store.clear(chat_id: chat_id) if draft&.origin == :voice + @idea_draft_store.clear_voice_draft(chat_id: chat_id) end def stage_voice_fallback(chat_id:, bytes:) @@ -812,17 +797,28 @@ def humanize_language(token) def execute_idea_commit(result, update) chat_id = result.attachment&.fetch(:chat_id, update.chat_id) || update.chat_id - draft = @idea_draft_store.get(chat_id: chat_id) - return safe_send_message(chat_id: update.chat_id, text: "That idea draft expired. Send /idea again.") unless draft - return safe_send_message(chat_id: update.chat_id, text: "Pick a project before pressing Done.") if draft.project.to_s.empty? - return safe_send_message(chat_id: update.chat_id, text: "Send the idea text before pressing Done.") if draft.text.to_s.strip.empty? + # Validation and execution both go through the store's decision API: + # commit_blocker owns the "is this committable" decision, and + # commit_snapshot hands execution a frozen view so it never depends on + # the live mutable Draft representation. + case @idea_draft_store.commit_blocker(chat_id: chat_id) + when :draft_expired + return safe_send_message(chat_id: update.chat_id, text: "That idea draft expired. Send /idea again.") + when :project_missing + return safe_send_message(chat_id: update.chat_id, text: "Pick a project before pressing Done.") + when :text_missing + return safe_send_message(chat_id: update.chat_id, text: "Send the idea text before pressing Done.") + end + + snapshot = @idea_draft_store.commit_snapshot(chat_id: chat_id) + return safe_send_message(chat_id: update.chat_id, text: "That idea draft expired. Send /idea again.") unless snapshot - tuples = draft.attachments.map { |attachment| [ attachment.fetch(:staging_path), attachment.fetch(:dest_name) ] } - body = tuples.empty? ? nil : idea_body_override(draft) + tuples = snapshot.attachments.map { |attachment| [ attachment.fetch(:staging_path), attachment.fetch(:dest_name) ] } + body = tuples.empty? ? nil : idea_body_override(snapshot) capture_command_io do Hive::Commands::New.new( - draft.project, - draft.text, + snapshot.project, + snapshot.text, body_override: body, attachments: tuples ).call! @@ -833,7 +829,7 @@ def execute_idea_commit(result, update) # is gone by design) — reads like an error for a successful capture. clear_inline_keyboard(update) if result.respond_to?(:clear_keyboard) && result.clear_keyboard safe_send_message(chat_id: update.chat_id, - text: "Captured your idea in #{draft.project}. It's in the inbox - move it to 2-brainstorm to start.") + text: "Captured your idea in #{snapshot.project}. It's in the inbox - move it to 2-brainstorm to start.") rescue Hive::Error, SystemCallError, IOError => e @logger.event(:send_failure, source: "commit_idea", chat_id: update.chat_id, @@ -843,8 +839,8 @@ def execute_idea_commit(result, update) text: "Couldn't capture that idea (#{Hive::Tui::Text.sanitize(e.message)}). Try Done again.") end - def idea_body_override(draft) - lines = draft.attachments.map do |attachment| + def idea_body_override(snapshot) + lines = snapshot.attachments.map do |attachment| dest = attachment.fetch(:dest_name) if Hive::Bot::IdeaAttachmentPolicy.image_extension?(attachment.fetch(:ext)) "![](assets/#{dest})" @@ -852,7 +848,7 @@ def idea_body_override(draft) "[#{dest}](assets/#{dest})" end end - ([ draft.text.to_s.strip ] + lines).reject(&:empty?).join("\n\n") + ([ snapshot.text.to_s.strip ] + lines).reject(&:empty?).join("\n\n") end def capture_command_io diff --git a/lib/hive/brainstorm_parser.rb b/lib/hive/brainstorm_parser.rb index 76cbb39baa..53a4246209 100644 --- a/lib/hive/brainstorm_parser.rb +++ b/lib/hive/brainstorm_parser.rb @@ -11,8 +11,24 @@ module Hive # daemon (gating auto-resume until every question is answered) need it. # `Hive::Bot::BrainstormParser` remains as a back-compat alias. module BrainstormParser + # Document location fields: + # + # - `question_line_index` — 0-based index of the `### Q{n}.` header line + # in the parser's newline-normalized line array (see `document_lines`). + # - `block_end_index` — 0-based index of the first block-boundary line + # (next `### Q`, `## Round`, or ``) at-or-after the Q + # header, or the line count at EOF. This is the exclusive end of the + # Q-block span and the position where a writer may insert a new A slot. + # - `answer_line_index` — 0-based index of the `### A{n}.` header line, + # or nil when the question has no A header on disk. + # + # These are the single source of truth for "where is this question in + # the document". Mutation callers (BrainstormAnswerWriter) must use + # them instead of re-scanning raw lines, so location rules cannot + # drift between the parser and writers. Question = Struct.new( :round, :n, :text, :answer, :answer_encoding, + :question_line_index, :block_end_index, :answer_line_index, keyword_init: true ) do def answered? @@ -56,9 +72,9 @@ def parse_text(text) current = nil mode = nil - normalize_newlines(text).each_line(chomp: true) do |line| + document_lines(text).each_with_index do |line, line_index| if (match = ROUND_RE.match(line)) - finalize(questions, current) + finalize(questions, current, line_index) current = nil mode = nil current_round = match[1].to_i @@ -66,12 +82,15 @@ def parse_text(text) end if (match = QUESTION_RE.match(line)) - finalize(questions, current) + finalize(questions, current, line_index) current = { round: current_round, n: match[1].to_i, question_lines: [ match[2] ], - answer_lines: nil + answer_lines: nil, + question_line_index: line_index, + block_end_index: nil, + answer_line_index: nil } mode = :question next @@ -94,6 +113,7 @@ def parse_text(text) if current && current[:answer_lines].nil? current[:answer_lines] = [] current[:answer_encoding] = :v1 if match[2] + current[:answer_line_index] = line_index mode = :answer end next @@ -103,7 +123,15 @@ def parse_text(text) # question presented to an operator or its fingerprint. In particular, # a final question with no A-header must keep the same fingerprint after # the writer repairs its slot immediately before ``. - next if MARKER_RE.match?(line) + if MARKER_RE.match?(line) + # A marker is a physical block boundary (same rule the writer's + # `block_boundary?` used to re-derive): a created A slot must + # land before it, never after. Record it once as the current + # block's end so downstream mutation callers get the identical + # span without duplicating the boundary rule. + current[:block_end_index] ||= line_index if current + next + end case mode when :question @@ -113,7 +141,7 @@ def parse_text(text) end end - finalize(questions, current) + finalize(questions, current, document_lines(text).length) # Preserve the file's physical order. Question numbers restart between # rounds and malformed-but-recoverable files can contain non-monotonic # numbering; callers that bind a user reply need the actual document @@ -121,6 +149,16 @@ def parse_text(text) questions end + # The newline-normalized line array that `parse_text` indexes its + # location fields (`question_line_index`, `block_end_index`, + # `answer_line_index`) against. Mutation callers must do line surgery + # on THIS array — not on `content.lines` — so parser indices and raw + # lines can never disagree about what a "line" is (notably for files + # containing lone `\r` line breaks). + def document_lines(text) + normalize_newlines(text).each_line(chomp: true).to_a + end + def next_unanswered_question(parsed) parsed.find { |question| question.answer.nil? } end @@ -156,7 +194,7 @@ def encoded_answer_header(n) "#{answer_header(n)} #{ANSWER_ENCODING_V1}" end - def finalize(out, current) + def finalize(out, current, block_end_index = nil) return unless current out << Question.new( @@ -164,7 +202,10 @@ def finalize(out, current) n: current[:n], text: clean_body(current[:question_lines]), answer: clean_answer(current[:answer_lines], encoding: current[:answer_encoding]), - answer_encoding: current[:answer_encoding] + answer_encoding: current[:answer_encoding], + question_line_index: current[:question_line_index], + block_end_index: current[:block_end_index] || block_end_index, + answer_line_index: current[:answer_line_index] ) end private_class_method :finalize diff --git a/lib/hive/stages/review/fix_guardrail.rb b/lib/hive/stages/review/fix_guardrail.rb index 748a4668fd..e939f616a2 100644 --- a/lib/hive/stages/review/fix_guardrail.rb +++ b/lib/hive/stages/review/fix_guardrail.rb @@ -144,11 +144,14 @@ def scan_diff(diff, patterns) diff.each_line do |line| chomped = line.chomp - # Reset current_file at the start of every file pair so a + # Re-seed current_file at the start of every file pair so a # subsequent +++ /dev/null (deletion) doesn't carry the - # previous file's path forward. + # previous file's path forward. Seed it from the header's own + # paths: mode lines (`new file mode 100755`) come BEFORE the + # ---/+++ pair, and a pure chmod emits no pair at all, so + # without this every permission_change finding had no file. if chomped.start_with?("diff --git ") - current_file = nil + current_file = diff_git_path(chomped.delete_prefix("diff --git ")) # Don't `next` — fall through so other targets (e.g. # raw_diff_header for permission_change) can still match # on the diff-git header line. @@ -212,6 +215,7 @@ def scan_diff(diff, patterns) patterns.each do |name, spec| next unless spec[:targets] == :raw_diff_header next unless spec[:regex] =~ chomped + next if exempt_new_file?(spec, chomped, current_file) matches << build_match( pattern_name: name.to_s, @@ -289,6 +293,28 @@ def add_file_path_matches(matches, patterns, path) end end + # Path of a `diff --git a/ b/` header, preferring the new + # side. Each side is C-quoted independently when it holds control + # characters. Unquoted paths may contain spaces, so an unchanged + # path is split symmetrically (backreference) before falling back + # to the last ` b/` for renames. + def diff_git_path(paths) + match = paths.match(%r{\A"[aciow]/.+" "[bciow]/(.+)"\z}) || + paths.match(%r{\A[aciow]/(.+) [bciow]/\1\z}) || + paths.match(%r{.* "?[bciow]/(.+?)"?\z}) + match && decode_git_path(match[1]) + end + + # A pattern may exempt NEW files under matching paths (e.g. a fake + # CLI stub under test/ must be executable to shadow the real one + # on PATH). Mode flips on existing files are never exempt. + def exempt_new_file?(spec, header, path) + exempt = spec[:exempt_new_file_paths] + return false unless exempt && path && header.start_with?("new file mode ") + + exempt.match?(path) + end + # Git C-quotes a path containing control characters (tab, newline, # quote, …) as `"path\tname"` regardless of core.quotePath, which # only covers non-ASCII bytes. Strip the surrounding double quotes @@ -318,7 +344,8 @@ def build_match(pattern_name:, file:, line:, snippet:, severity:, match_sha256: Digest::SHA256.hexdigest(snippet.to_s)) Match.new(pattern_name:, file:, line:, snippet:, severity:, match_sha256:) end - private_class_method :add_file_path_matches, :decode_git_path, :build_match + private_class_method :add_file_path_matches, :diff_git_path, :exempt_new_file?, + :decode_git_path, :build_match end end end diff --git a/lib/hive/stages/review/fix_guardrail/patterns.rb b/lib/hive/stages/review/fix_guardrail/patterns.rb index 59d5d069b6..9b5e473431 100644 --- a/lib/hive/stages/review/fix_guardrail/patterns.rb +++ b/lib/hive/stages/review/fix_guardrail/patterns.rb @@ -28,6 +28,8 @@ module Patterns # :severity — :high | :medium | :nit (used to group findings in fix-guardrail-NN.md) # :targets — :code | :file_path (which side of the diff to scan) # :description — single-line explanation surfaced in the finding + # :exempt_new_file_paths — optional Regexp; a :raw_diff_header match on a + # `new file mode` line is skipped when the file path matches DEFAULTS = { shell_pipe_to_interpreter: { detector: :regex, @@ -91,6 +93,11 @@ module Patterns regex: /\A(?:old mode|new mode|deleted file mode|new file mode) 10[0-9][0-9][0-9][1357]$/, severity: :medium, targets: :raw_diff_header, + # New executables under test directories are fake CLI stubs + # and fixture scripts (e.g. web/test/e2e/support/codex), not + # granted execution rights. chmod +x of an existing file, or a + # new executable anywhere else, still trips. + exempt_new_file_paths: %r{(?:\A|/)(?:test|tests|spec|specs|__tests__|testdata|fixtures?)/}, description: "executable / setuid / setgid bit added: a fix that flips file mode to an executable or privileged mode may be granting execution rights to a script the user didn't expect." } }.freeze diff --git a/templates/review_prompt.md.erb b/templates/review_prompt.md.erb index d4d6dd1e21..e6e8c6c370 100644 --- a/templates/review_prompt.md.erb +++ b/templates/review_prompt.md.erb @@ -8,7 +8,7 @@ Pass: <%= pass %> Behavior: 1. Use the `/ce-code-review` skill on the diff - `git diff <%= default_branch %>..HEAD` in the current worktree. + `git diff <%= default_branch %>...HEAD` in the current worktree. 2. Output structured findings to <%= task_folder %>/reviews/ce-review-<%= "%02d" % pass %>.md using GFM checkboxes grouped by severity: diff --git a/templates/reviewer_claude_ce_code_review.md.erb b/templates/reviewer_claude_ce_code_review.md.erb index 460e6b82c5..813900c04f 100644 --- a/templates/reviewer_claude_ce_code_review.md.erb +++ b/templates/reviewer_claude_ce_code_review.md.erb @@ -9,7 +9,7 @@ Pass: <%= pass %> Behavior: 1. Use the `<%= skill_invocation %>` skill on the diff - `git diff <%= default_branch %>..HEAD` in the current worktree. + `git diff <%= default_branch %>...HEAD` in the current worktree. 2. Output structured findings to <%= output_path %> using GFM checkboxes grouped by severity: diff --git a/templates/reviewer_codex_ce_code_review.md.erb b/templates/reviewer_codex_ce_code_review.md.erb index 460e6b82c5..813900c04f 100644 --- a/templates/reviewer_codex_ce_code_review.md.erb +++ b/templates/reviewer_codex_ce_code_review.md.erb @@ -9,7 +9,7 @@ Pass: <%= pass %> Behavior: 1. Use the `<%= skill_invocation %>` skill on the diff - `git diff <%= default_branch %>..HEAD` in the current worktree. + `git diff <%= default_branch %>...HEAD` in the current worktree. 2. Output structured findings to <%= output_path %> using GFM checkboxes grouped by severity: diff --git a/templates/reviewer_grok_ce_code_review.md.erb b/templates/reviewer_grok_ce_code_review.md.erb index 92a1a04e42..a25542282f 100644 --- a/templates/reviewer_grok_ce_code_review.md.erb +++ b/templates/reviewer_grok_ce_code_review.md.erb @@ -9,7 +9,7 @@ Pass: <%= pass %> Behavior: 1. Use the `<%= skill_invocation %>` skill on the diff - `git diff <%= default_branch %>..HEAD` in the current worktree. + `git diff <%= default_branch %>...HEAD` in the current worktree. 2. Output structured findings to <%= output_path %> using GFM checkboxes grouped by severity: diff --git a/templates/reviewer_pr_review_toolkit.md.erb b/templates/reviewer_pr_review_toolkit.md.erb index 4a2fc9a0e5..6ee550cd1b 100644 --- a/templates/reviewer_pr_review_toolkit.md.erb +++ b/templates/reviewer_pr_review_toolkit.md.erb @@ -9,7 +9,7 @@ Pass: <%= pass %> Behavior: 1. Use the `<%= skill_invocation %>` skill on the diff - `git diff <%= default_branch %>..HEAD` in the current worktree. + `git diff <%= default_branch %>...HEAD` in the current worktree. pr-review-toolkit runs multi-persona review internally; capture the union of all persona findings. 2. Output structured findings to <%= output_path %> diff --git a/test/eval/support/reporter_test.rb b/test/eval/support/reporter_test.rb index 2e070d4261..6f3a8e8002 100644 --- a/test/eval/support/reporter_test.rb +++ b/test/eval/support/reporter_test.rb @@ -54,6 +54,28 @@ def test_cli_rejects_inherited_rake_dry_run end end + def test_cli_rejects_inherited_rake_dry_run_clears_stale_default_report + root = File.expand_path("../../..", __dir__) + default_report = File.join(root, "tmp", "hive-eval-report.json") + FileUtils.mkdir_p(File.dirname(default_report)) + preserve_path(default_report) do + File.write(default_report, "stale default report\n") + + [ "-n", "--dry-run", "--bogus" ].each do |rakeopt| + _out, err, status = Open3.capture3( + { "HIVE_EVAL_NO_JUDGE" => "1", "RAKEOPT" => rakeopt }, + "bin/hive-eval", "--no-judge" + ) + + refute status.success?, "RAKEOPT=#{rakeopt.inspect} must not turn a skipped eval into success" + assert_match(/RAKEOPT/, err) + refute File.exist?(default_report), + "RAKEOPT=#{rakeopt.inspect}: a rejected inherited RAKEOPT must invalidate a stale default report" + File.write(default_report, "stale default report\n") + end + end + end + def test_cli_clears_inherited_rake_options_from_child [ "--trace", "-Iinclude", "-Wno", "-fn" ].each do |rakeopt| Dir.mktmpdir("hive-eval-report") do |dir| diff --git a/test/unit/bot/brainstorm_answer_writer_test.rb b/test/unit/bot/brainstorm_answer_writer_test.rb index f6f5eb5158..213130f738 100644 --- a/test/unit/bot/brainstorm_answer_writer_test.rb +++ b/test/unit/bot/brainstorm_answer_writer_test.rb @@ -369,18 +369,13 @@ def test_write_at_ordinal_under_existing_lock_targets_later_same_number end end - def test_exact_writer_rejects_invalid_ordinals_and_missing_raw_question_positions + def test_exact_writer_rejects_invalid_ordinals with_brainstorm(sample) do |path| assert_equal :question_not_found, Hive::Bot::BrainstormAnswerWriter.write_at_ordinal_under_lock!( brainstorm_path: path, ordinal: Object.new, answer_text: "never written" ) end - - lines = [ "## Round 1\n", "### Q1. Only question?\n" ] - assert_nil Hive::Bot::BrainstormAnswerWriter.send( - :question_line_index_for_ordinal, lines, 2 - ) end # Boundary coverage (#269): the slot-creation scan must STOP at a @@ -464,24 +459,69 @@ def test_creates_slot_when_question_has_no_trailing_newline end end - # Defensive: if parsed says Q{n} is unanswered but the raw lines no - # longer contain that Q (e.g. brainstorm.md was rewritten between - # `parse_text` and `lines = content.lines` — impossible under the - # same `with_task_lock`, but the code guards it anyway), the - # writer must return nil → caller surfaces :answer_slot_missing - # rather than misattributing to a different line. Exercises the - # defensive return at the end of target_question_line_index. - def test_target_question_line_index_returns_nil_when_parsed_diverges_from_lines - fake_parsed = [ - Hive::Bot::BrainstormParser::Question.new(round: 1, n: 1, text: "synthetic", answer: nil) - ] - # Lines contain NO `### Q1.` header at all — simulates a parse-vs-lines - # divergence that would otherwise let the writer misattribute. - lines = [ "## Round 1\n", "\n", "(no Q1 line on disk)\n" ] - - result = Hive::Bot::BrainstormAnswerWriter.send(:target_question_line_index, lines, fake_parsed, 1) - assert_nil result, - "defensive nil must fire when parsed and raw lines disagree about Q{n}" + # Mutation calls anchor on the parser's location fields (`question_line_index`, + # `block_end_index`, `answer_line_index`) instead of re-deriving line indices + # from raw lines, so a parse-vs-lines divergence is structurally impossible + # (one line array, one index space). The old defensive re-scan + # (`target_question_line_index` returning nil on divergence) was replaced by + # anchoring directly on `Question` location fields; this test pins the parser + # output the writer relies on. + def test_writer_anchors_on_parser_location_fields + text = "## Round 1\n\n### Q1. First?\n\n### A1.\n\n### Q2. Second?\n\n### A2.\n" + + parsed = Hive::Bot::BrainstormParser.parse_text(text) + target = parsed.find { |question| question.n == 2 && question.answer.nil? } + + refute_nil target + assert_equal 6, target.question_line_index, + "the writer fills/creates the slot at the parser-identified Q header line" + assert_equal 9, target.block_end_index, + "the writer inserts before the parser-identified block end (EOF, after the A2 body)" + assert_equal 8, target.answer_line_index, + "the writer fills the empty A-body between the A header and the block end" + end + + def test_append_writes_answer_into_lone_cr_file + # BrainstormAnswerWriter.append! used to dead-end in + # :answer_slot_missing on a lone-\r document: parse_text normalized + # lone \r into line breaks while `content.lines` did not, so the + # writer-side raw-line re-scan could never locate the target Q line. + # The writer now does line surgery on the parser's own + # document_lines array, so the two views cannot diverge. (The exact + # ordinal writer always normalized first; that behavior is pinned by + # test_exact_writer_maps_ordinals_in_lone_cr_files.) + content = "## Round 1\r### Q1. First?\r### A1.\r### Q2. Second?\r### A2.\r\r" + with_brainstorm(content) do |path| + result = Hive::Bot::BrainstormAnswerWriter.append!( + brainstorm_path: path, + question_n: 1, + answer_text: "first" + ) + + assert_equal :written, result + parsed = Hive::BrainstormParser.parse(path) + assert_equal "first", parsed[0].answer + assert_nil parsed[1].answer + end + end + + def test_append_preserves_crlf_line_endings + content = "## Round 1\r\n\r\n### Q1. First?\r\n\r\n### A1.\r\n\r\n### Q2. Second?\r\n\r\n### A2.\r\n" + with_brainstorm(content) do |path| + result = Hive::Bot::BrainstormAnswerWriter.append!( + brainstorm_path: path, + question_n: 1, + answer_text: "first" + ) + + assert_equal :written, result + raw = File.read(path) + # Byte-identical to the pre-refactor writer: the blank line between the + # empty A body and the Q2 boundary was (and still is) part of the empty + # slot envelope and is replaced by the answer. + assert_equal "## Round 1\r\n\r\n### Q1. First?\r\n\r\n### A1. #{Hive::BrainstormParser::ANSWER_ENCODING_V1}\r\nfirst\r\n### Q2. Second?\r\n\r\n### A2.\r\n", raw + assert_equal "first", Hive::BrainstormParser.parse(path)[0].answer + end end # F2 from PR #239 ce-code-review: when try_append returns :enoent diff --git a/test/unit/bot/brainstorm_parser_test.rb b/test/unit/bot/brainstorm_parser_test.rb index 8ccb7ad0c9..41351f9ded 100644 --- a/test/unit/bot/brainstorm_parser_test.rb +++ b/test/unit/bot/brainstorm_parser_test.rb @@ -204,6 +204,34 @@ def test_questions_preserve_physical_document_order assert_equal 3, Hive::Bot::BrainstormParser.next_unanswered_question(questions).n end + # Location fields are the parser's source of truth for document + # positions. Mutation callers (BrainstormAnswerWriter) use them instead + # of re-deriving line indices from raw lines, so parser and writer can + # never disagree about where a question block starts, where it ends, + # and where its A-header sits. + def test_exposes_document_location_fields + text = "## Round 1\n\n### Q1. First?\n\n### A1.\n\n### Q2. Second?\n\n\n" + + questions = Hive::Bot::BrainstormParser.parse_text(text) + + q1, q2 = questions + assert_equal 2, q1.question_line_index + assert_equal 6, q1.block_end_index + assert_equal 4, q1.answer_line_index + assert_equal 6, q2.question_line_index + assert_equal 8, q2.block_end_index + assert_nil q2.answer_line_index + end + + def test_block_end_index_points_at_eof_for_a_trailing_question + text = "## Round 1\n### Q1. Final?" + + questions = Hive::Bot::BrainstormParser.parse_text(text) + + assert_equal 1, questions.first.question_line_index + assert_equal 2, questions.first.block_end_index + end + def test_question_fingerprint_normalizes_unicode_and_layout_but_not_wording compact = Hive::BrainstormParser.question_fingerprint("Use the café path?") reformatted = Hive::BrainstormParser.question_fingerprint(" Use\tthe cafe\u0301\npath? ") diff --git a/test/unit/bot/idea_draft_store_test.rb b/test/unit/bot/idea_draft_store_test.rb index 9ee4920243..6cbec8cdc9 100644 --- a/test/unit/bot/idea_draft_store_test.rb +++ b/test/unit/bot/idea_draft_store_test.rb @@ -147,4 +147,161 @@ def test_clear_swallows_staging_cleanup_failure assert_nil @store.get(chat_id: 1) end end + + # ---- State-decision API regression (encapsulated phase/origin decisions) ---- + + def test_awaiting_transcript_confirmation_is_true_only_for_voice_confirm_phase + @store.start(chat_id: 1, phase: :awaiting_text, token: "tok") + refute @store.awaiting_transcript_confirmation?(chat_id: 1), + "a plain typed draft must not route to the voice confirm flow" + + @store.start(chat_id: 1, phase: :awaiting_transcript_confirm, token: "tok", origin: :voice) + assert @store.awaiting_transcript_confirmation?(chat_id: 1) + + @now += 901 + refute @store.awaiting_transcript_confirmation?(chat_id: 1), + "an expired draft must not route to the voice confirm flow" + end + + def test_voice_and_non_voice_draft_predicates_split_on_origin + @store.start(chat_id: 1, phase: :awaiting_text, token: "tok") + assert @store.non_voice_draft?(chat_id: 1) + refute @store.voice_draft?(chat_id: 1) + + @store.start(chat_id: 1, phase: :awaiting_transcript_confirm, token: "tok2", origin: :voice) + assert @store.voice_draft?(chat_id: 1) + refute @store.non_voice_draft?(chat_id: 1) + + @now += 901 + refute @store.voice_draft?(chat_id: 1), "expired drafts must count as absent" + refute @store.non_voice_draft?(chat_id: 1) + end + + def test_awaiting_text_draft_is_true_only_in_awaiting_text_phase + @store.start(chat_id: 1, phase: :awaiting_text, token: "tok") + assert @store.awaiting_text_draft?(chat_id: 1) + + @store.set_text(chat_id: 1, text: "fix login") + refute @store.awaiting_text_draft?(chat_id: 1) + end + + def test_transcript_only_voice_draft_requires_voice_origin_and_no_audio + @store.start(chat_id: 1, phase: :awaiting_project, text: "idea", token: "tok") + refute @store.transcript_only_voice_draft?(token: "tok"), + "a typed draft must enter file collection, not commit immediately" + + @store.start(chat_id: 1, phase: :awaiting_transcript_confirm, token: "tok2", origin: :voice) + @store.confirm_transcript(chat_id: 1) + assert @store.transcript_only_voice_draft?(token: "tok2") + + @store.append_attachment(chat_id: 1, label: "voice-1", dest_name: "voice-1.oga", + staging_path: "/tmp/voice-1.oga", ext: "oga") + refute @store.transcript_only_voice_draft?(token: "tok2"), + "staged fallback audio must divert the draft into file collection" + end + + def test_ensure_voice_draft_reuses_only_voice_and_never_clobbers_non_voice + @store.start(chat_id: 1, phase: :awaiting_project, text: "typed idea", token: "tok") + assert_nil @store.ensure_voice_draft(chat_id: 1, token: "vtok"), + "a non-voice draft must not be reused or replaced by a voice note" + assert_equal "tok", @store.get(chat_id: 1).token + + @store.start(chat_id: 1, phase: :awaiting_transcript_confirm, token: "tok2", origin: :voice) + reused = @store.ensure_voice_draft(chat_id: 1, token: "tok3") + assert_equal "tok2", reused.token, "a live voice draft must be reused" + + @store.clear(chat_id: 1) + fresh = @store.ensure_voice_draft(chat_id: 1, token: "tok4") + assert_equal "tok4", @store.get(chat_id: 1).token + assert_equal :awaiting_transcript_confirm, fresh.phase + assert_equal :voice, fresh.origin + end + + def test_clear_voice_draft_leaves_non_voice_drafts_alone + @store.start(chat_id: 1, phase: :collecting_files, text: "typed", token: "tok") + @store.clear_voice_draft(chat_id: 1) + refute_nil @store.get(chat_id: 1), "a text/media draft must survive a rejected voice note" + + @store.start(chat_id: 1, phase: :awaiting_transcript_confirm, token: "tok2", origin: :voice) + @store.clear_voice_draft(chat_id: 1) + assert_nil @store.get(chat_id: 1) + end + + def test_commit_blocker_reports_each_missing_requirement + assert_equal :draft_expired, @store.commit_blocker(chat_id: 1) + + @store.start(chat_id: 1, phase: :awaiting_project, text: "idea", token: "tok") + assert_equal :project_missing, @store.commit_blocker(chat_id: 1) + + @store.set_project(chat_id: 1, project: "hive") + assert_nil @store.commit_blocker(chat_id: 1) + + @store.start(chat_id: 2, phase: :awaiting_project, text: " ", token: "tok2") + @store.set_project(chat_id: 2, project: "hive") + assert_equal :text_missing, @store.commit_blocker(chat_id: 2), + "whitespace-only text must block commit" + end + + def test_commit_snapshot_returns_frozen_view_independent_of_live_draft + @store.start(chat_id: 1, phase: :awaiting_project, text: "idea", token: "tok") + @store.set_project(chat_id: 1, project: "hive") + @store.append_attachment(chat_id: 1, label: "bug-1", dest_name: "bug-1.jpg", + staging_path: "/tmp/bug-1.jpg", ext: "jpg") + + snapshot = @store.commit_snapshot(chat_id: 1) + assert_equal "hive", snapshot.project + assert_equal "idea", snapshot.text + assert_equal [ { staging_path: "/tmp/bug-1.jpg", dest_name: "bug-1.jpg", ext: "jpg" } ], snapshot.attachments + + # The snapshot is a frozen execution view: mutating the live draft (or the + # snapshot) afterwards must not change what was already handed to commit. + @store.append_attachment(chat_id: 1, label: "bug-2", dest_name: "bug-2.pdf", + staging_path: "/tmp/bug-2.pdf", ext: "pdf") + assert_equal 1, snapshot.attachments.size + + assert_raises(FrozenError) { snapshot.attachments << {} } + assert_raises(FrozenError) { snapshot.project = "other" } + + assert_nil @store.commit_snapshot(chat_id: 999), "no draft means no snapshot" + end + + def test_commit_snapshot_string_leaves_survive_live_draft_mutation + @store.start(chat_id: 1, phase: :awaiting_project, text: "idea", token: "tok", origin: :voice) + @store.set_project(chat_id: 1, project: "hive") + @store.append_attachment(chat_id: 1, label: "bug-1", dest_name: "bug-1.jpg", + staging_path: "/tmp/bug-1.jpg", ext: "jpg") + + snapshot = @store.commit_snapshot(chat_id: 1) + + # Frozen containers alone are not an independent view: freeze does not + # deep-freeze, so the String leaves must each be duplicated and frozen, + # or in-place edits on the live draft rewrite what execution received. + draft = @store.get(chat_id: 1) + draft.text << "-mutated" + draft.project.replace("hive-mutated") + attachment = draft.attachments.first + attachment[:staging_path] << "-moved" + attachment[:dest_name].replace("evil.jpg") + + assert_equal "idea", snapshot.text, "snapshot text must not alias the live draft's text" + assert snapshot.text.frozen? + assert_equal "hive", snapshot.project, "snapshot project must not alias the live draft's project" + assert snapshot.project.frozen? + attachment_view = snapshot.attachments.first + assert_equal "/tmp/bug-1.jpg", attachment_view[:staging_path] + assert attachment_view[:staging_path].frozen? + assert_equal "bug-1.jpg", attachment_view[:dest_name] + assert attachment_view[:dest_name].frozen? + assert_equal "jpg", attachment_view[:ext] + assert attachment_view[:ext].frozen? + end + + def test_commit_snapshot_passes_nil_text_and_project_through + @store.start(chat_id: 1, phase: :awaiting_text, token: "tok") + + snapshot = @store.commit_snapshot(chat_id: 1) + + assert_nil snapshot.text + assert_nil snapshot.project + end end diff --git a/test/unit/bot/supervisor_test.rb b/test/unit/bot/supervisor_test.rb index 5fce2b9ee2..6848a0122d 100644 --- a/test/unit/bot/supervisor_test.rb +++ b/test/unit/bot/supervisor_test.rb @@ -1164,6 +1164,31 @@ def test_commit_idea_replies_for_missing_project_and_missing_text @idea_draft_store.clear(chat_id: 42) if @idea_draft_store end + def test_commit_idea_replies_when_draft_expires_between_validation_and_snapshot + @idea_draft_store.start(chat_id: 42, phase: :collecting_files, text: "fix", token: "tok") + @idea_draft_store.set_project(chat_id: 42, project: "hive") + @idea_draft_store.define_singleton_method(:commit_snapshot) { |chat_id:| nil } + + @supervisor.send(:execute_result, + FakeRouter::Result.new(action: :commit_idea, attachment: { chat_id: 42 }), + Update.new(chat_id: 42, update_id: 1)) + + assert_match(/idea draft expired/, @telegram.messages.last.fetch(:text)) + ensure + @idea_draft_store.clear(chat_id: 42) if @idea_draft_store + end + + def test_commit_idea_replies_when_draft_is_already_expired + @idea_draft_store.define_singleton_method(:commit_blocker) { |chat_id:| :draft_expired } + @idea_draft_store.define_singleton_method(:commit_snapshot) { |chat_id:| flunk "must not snapshot an expired draft" } + + @supervisor.send(:execute_result, + FakeRouter::Result.new(action: :commit_idea, attachment: { chat_id: 42 }), + Update.new(chat_id: 42, update_id: 1)) + + assert_match(/idea draft expired/, @telegram.messages.last.fetch(:text)) + end + def test_commit_idea_handles_command_failure_without_clearing_draft @idea_draft_store.start(chat_id: 42, phase: :collecting_files, text: "fix", token: "tok") @idea_draft_store.set_project(chat_id: 42, project: "missing-project") diff --git a/test/unit/reviewers/agent_test.rb b/test/unit/reviewers/agent_test.rb index 5708a60f4e..f61fdfae95 100644 --- a/test/unit/reviewers/agent_test.rb +++ b/test/unit/reviewers/agent_test.rb @@ -883,7 +883,8 @@ def test_argv_invokes_claude_with_expected_skill_in_prompt # The rendered prompt is the last positional argv arg. assert_includes argv, "/ce-code-review", "prompt must invoke /ce-code-review skill" assert_includes argv, ctx.task_folder, "prompt must mention the task folder" - assert_includes argv, "git diff main..HEAD", "prompt must invoke the diff against the default branch" + assert_includes argv, "git diff main...HEAD", + "prompt must invoke the merge-base diff against the default branch" ensure FileUtils.rm_rf(log_dir) if log_dir end diff --git a/test/unit/stages/review/fix_guardrail_test.rb b/test/unit/stages/review/fix_guardrail_test.rb index 8e4cf0b628..a3cd51b0fc 100644 --- a/test/unit/stages/review/fix_guardrail_test.rb +++ b/test/unit/stages/review/fix_guardrail_test.rb @@ -1026,9 +1026,84 @@ def test_trips_on_executable_permission_change refute_nil match assert_match(/new file mode 100755|new mode 100755/, match.snippet) assert_equal :medium, match.severity + assert_equal "scripts/run.sh", match.file, + "mode lines precede the ---/+++ pair; the path must come from the diff --git header" end end + def test_new_executable_stub_under_test_directory_is_exempt + # A fix pass that adds a fake CLI stub for an E2E test (the stub must be + # executable to shadow the real binary on PATH) is not a privilege grant. + with_two_commits(file: "web/test/e2e/support/codex", + content: "#!/usr/bin/env bash\nexit 0\n", + mode: 0o755) do |dir, base, head| + result = Hive::Stages::Review::FixGuardrail.run!( + cfg: cfg, ctx: make_ctx(dir), + base_sha: base, head_sha: head + ) + + assert_equal :clean, result.status + end + end + + def test_chmod_of_existing_test_file_still_trips_with_its_path + # The exemption covers NEW files only. A pure mode flip emits no ---/+++ + # pair at all, so the diff --git header is the only source of the path. + with_two_commits(file: "test/helper.sh", content: "echo hi\n") do |dir, _base, _head| + base = `git -C #{dir} rev-parse HEAD`.strip + File.chmod(0o755, File.join(dir, "test/helper.sh")) + run!("git", "-C", dir, "commit", "-am", "chmod", "--quiet") + head = `git -C #{dir} rev-parse HEAD`.strip + + result = Hive::Stages::Review::FixGuardrail.run!( + cfg: cfg, ctx: make_ctx(dir), + base_sha: base, head_sha: head + ) + + assert_equal :tripped, result.status + match = result.matches.find { |m| m.pattern_name == "permission_change" } + assert_equal "new mode 100755", match.snippet + assert_equal "test/helper.sh", match.file + end + end + + def test_renamed_executable_reports_the_new_path + with_two_commits(file: "old.sh", content: "#!/bin/sh\n#{"echo same\n" * 20}") do |dir, _base, _head| + base = `git -C #{dir} rev-parse HEAD`.strip + FileUtils.mkdir_p(File.join(dir, "bin")) + run!("git", "-C", dir, "mv", "old.sh", "bin/new.sh") + File.chmod(0o755, File.join(dir, "bin/new.sh")) + run!("git", "-C", dir, "commit", "-am", "rename", "--quiet") + head = `git -C #{dir} rev-parse HEAD`.strip + + result = Hive::Stages::Review::FixGuardrail.run!( + cfg: cfg, ctx: make_ctx(dir), + base_sha: base, head_sha: head + ) + + match = result.matches.find { |m| m.pattern_name == "permission_change" } + assert_equal "bin/new.sh", match.file + end + end + + def test_c_quoted_executable_path_is_decoded + with_two_commits(file: "tab\tname.sh", content: "#!/bin/sh\n", mode: 0o755) do |dir, base, head| + result = Hive::Stages::Review::FixGuardrail.run!( + cfg: cfg, ctx: make_ctx(dir), + base_sha: base, head_sha: head + ) + + match = result.matches.find { |m| m.pattern_name == "permission_change" } + assert_equal "tab\tname.sh", match.file + end + end + + def test_diff_git_path_splits_unquoted_paths_with_spaces + guardrail = Hive::Stages::Review::FixGuardrail + assert_equal "my dir/b x.sh", guardrail.send(:diff_git_path, "a/my dir/b x.sh b/my dir/b x.sh") + assert_nil guardrail.send(:diff_git_path, "garbage") + end + # --- permission_change pattern direct coverage ----------------------- def test_permission_change_regex_matches_100755_executable_bit diff --git a/test/unit/templates/reviewer_diff_range_test.rb b/test/unit/templates/reviewer_diff_range_test.rb new file mode 100644 index 0000000000..0ef1c01dc4 --- /dev/null +++ b/test/unit/templates/reviewer_diff_range_test.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require "test_helper" + +# Branch-only reviewer prompts must diff against the merge-base of the +# default branch and HEAD (`git diff ...HEAD`), never the two-dot +# tip-to-tip range (`git diff ..HEAD`). With two-dot, commits that +# landed on the default branch after the feature branch was cut appear in +# the diff as their inverse, so reviewers see unrelated "changes" that the +# branch never introduced. The three-dot merge-base range shows only what +# the branch itself added. +# +# Regression for patrol finding +# architecture-templates-part-8-20260917T000739Z-a272c010-1: the Grok, +# pr-review-toolkit, Claude, Codex CE, and legacy execute-review templates +# used two-dot notation while the Codex native reviewer already used +# three-dot. +class ReviewerDiffRangeTest < Minitest::Test + TEMPLATES = %w[ + reviewer_claude_ce_code_review.md.erb + reviewer_codex_ce_code_review.md.erb + reviewer_codex_native_review.md.erb + reviewer_grok_ce_code_review.md.erb + reviewer_pr_review_toolkit.md.erb + review_prompt.md.erb + ].freeze + + TEST_DIR = File.expand_path(__dir__) + + def test_reviewer_templates_diff_against_merge_base_not_tips + TEMPLATES.each do |name| + source = File.read(File.join(TEST_DIR, "../../../templates", name)) + + refute_includes source, "git diff <%= default_branch %>..HEAD", + "#{name} must not use the two-dot tip-to-tip range; " \ + "it includes inverse changes from the default branch" + assert_includes source, "git diff <%= default_branch %>...HEAD", + "#{name} must review the three-dot merge-base diff" + end + end +end diff --git a/wiki/log.d/20260917T003658Z-reviewer-merge-base-diff.md b/wiki/log.d/20260917T003658Z-reviewer-merge-base-diff.md new file mode 100644 index 0000000000..6451c02872 --- /dev/null +++ b/wiki/log.d/20260917T003658Z-reviewer-merge-base-diff.md @@ -0,0 +1,17 @@ +# Reviewer prompts use merge-base diff range + +Patrol finding `architecture-templates-part-8-20260917T000739Z-a272c010-1` +(generation 1): the agent reviewer templates (`reviewer_grok_ce_code_review`, +`reviewer_pr_review_toolkit`, plus the sibling `reviewer_claude_ce_code_review`, +`reviewer_codex_ce_code_review`, and legacy `review_prompt`) instructed +reviewers to run `git diff ..HEAD` — the two-dot tip-to-tip +range. When the default branch advanced after the feature branch was cut, that +range also emitted the inverse of the new default-branch commits, so reviewers +saw unrelated "changes" the branch never introduced. The codex native reviewer +already used the three-dot merge-base range (`...`) and was correct. + +All five affected templates now render `git diff ...HEAD`, so +every reviewer sees only the changes the branch itself added. +`test/unit/templates/reviewer_diff_range_test.rb` pins the merge-base notation +across all reviewer templates, and the +`Reviewers::Agent` argv test now asserts `git diff main...HEAD`. diff --git a/wiki/log.d/20260921T000000Z-brainstorm-document-location.md b/wiki/log.d/20260921T000000Z-brainstorm-document-location.md new file mode 100644 index 0000000000..ef392bc285 --- /dev/null +++ b/wiki/log.d/20260921T000000Z-brainstorm-document-location.md @@ -0,0 +1,67 @@ +--- +timestamp: 2026-09-21T000000Z +type: fix +pages: [modules/bot, testing] +tags: [bot, brainstorm, parser, architecture] +--- + +# Centralize brainstorm document location in the parser + +**Action:** The architecture patrol finding +`scheduled-…:centralize-brainstorm-document-location` flagged that +`Hive::BrainstormParser` returned only logical question data while +`BrainstormAnswerWriter` reconstructed physical identity from raw lines: +`target_question_line_index` re-scanned raw lines to map the parser's +"first unanswered Q{n}" back to a line index, +`question_line_index_for_ordinal` re-scanned for the ordinal-position Q +header, and `block_boundary?` re-derived the Q/Round/marker boundary rule +the parser also encodes. Three parallel location rules made every future +format change a synchronized cross-boundary edit, and the raw-line re-scan +indexed a different line array than the parser used (a lone-`\r` file +yields more parser lines than `content.lines`). + +Fixed by making the parser the single source of truth for document +location (`lib/hive/brainstorm_parser.rb`, +`lib/hive/bot/brainstorm_answer_writer.rb`): + +1. `Question` now carries `question_line_index`, `block_end_index` (first + Q/Round/marker boundary at-or-after the Q header, or EOF), and + `answer_line_index`, all indexed against `document_lines(text)` — the + parser's newline-normalized line array, newly exposed as a public + helper. +2. `find_empty_answer_slot` takes the parser-identified unanswered target + and returns the empty-slot envelope from `answer_line_index` / + `block_end_index`; slot creation inserts at `block_end_index`. The + writer-side re-scan helpers and `block_boundary?` are gone. +3. Writer line surgery operates on the parser's `document_lines` array + (re-terminated with `\n`), so parser indices and raw lines can never + disagree; the written document keeps the pre-write line-ending style + (CRLF stays CRLF, decided once by `join_document` instead of + per-inserted-line `newline_for`). + +Behavioral consequences: lone-`\r` documents now land answers through +`append!` instead of dead-ending in `:answer_slot_missing` (the parser and +the writer finally share one line array), while the lone-`\r` exact-writer +path keeps working; EOF-final questions keep their terminator. The +Q-context anchoring that fixed the earlier cross-round slot leak is now +structural rather than scan-based. + +**Tests:** focused suites `test/unit/bot/brainstorm_parser_test.rb`, +`test/unit/bot/brainstorm_answer_writer_test.rb`, +`test/integration/bot/brainstorm_round_trip_test.rb`, +`test/eval/scenarios/s6_brainstorm_round_trip_test.rb`, +`test/eval/scenarios/s2_agent_question_test.rb`, +`test/integration/bot/scenarios/s1_brainstorm_test.rb`, +`test/integration/brainstorm_answering_skill_contract_test.rb`, +`test/unit/commands/answer_test.rb`, plus +`brainstorm_runtime`/`brainstorm_artifact`/`tui/brainstorm_answers`/ +`auto_retry_safety`/`answer_digest`/`bot/supervisor` — all green. +Added parser location-field tests (`test_exposes_document_location_fields`, +`test_block_end_index_points_at_eof_for_a_trailing_question`), a +writer-anchoring regression test, and full-file CRLF and lone-CR +append-round-trip coverage. The obsolete +`target_question_line_index`/`question_line_index_for_ordinal` test hooks +were replaced. + +**Pages:** updated [[modules/bot]] (`BrainstormParser` location-source row, +`BrainstormAnswerWriter` anchoring row). diff --git a/wiki/log.d/20260922-encapsulate-idea-draft-state-decisions.md b/wiki/log.d/20260922-encapsulate-idea-draft-state-decisions.md new file mode 100644 index 0000000000..31c51a6f7d --- /dev/null +++ b/wiki/log.d/20260922-encapsulate-idea-draft-state-decisions.md @@ -0,0 +1,52 @@ +--- +title: Encapsulate idea-draft state decisions in IdeaDraftStore +date: 2026-09-22 +tags: [bot, idea-draft, encapsulation, patrol] +--- + +## Encapsulate idea-draft state decisions in IdeaDraftStore + +Patrol finding `encapsulate-idea-draft-state-decisions` (architecture patrol, +scheduled run `scheduled-dad16bf…`): `IdeaDraftStore` declared that the +phase/origin contract belongs to the store, but it still handed callers a live +mutable `Draft` Struct, and three consumers re-derived compound state +interpretations from raw fields: + +- `Router` read `phase == :awaiting_transcript_confirm && origin == :voice` + for transcript-confirm routing and `origin != :voice` for the + voice-during-draft short-circuit, plus `phase == :awaiting_text` for idea + text capture. +- `CallbackHandlers#collect_files_for_draft` owned a second compound + interpretation (`origin == :voice && attachments.empty?`) to choose + immediate commit versus file collection. +- `Supervisor` independently validated project/text, consumed attachment + tuples, and branched on origin (`non_voice_draft?`, `ensure_voice_draft`, + `clear_voice_draft`, `execute_idea_commit`), making execution depend on the + store's mutable representation. + +### Change + +`lib/hive/bot/idea_draft_store.rb` now exposes a state-decision API that is +the single authority for these interpretations: + +- Routing/callback predicates: `awaiting_transcript_confirmation?`, + `non_voice_draft?`, `voice_draft?`, `awaiting_text_draft?`, + `transcript_only_voice_draft?(token:)`. +- Voice reuse/reject/clear transitions moved into the store: + `ensure_voice_draft(chat_id:, token:)` (reuse voice-origin only, start + fresh when empty, return nil for an open text/media draft) and + `clear_voice_draft(chat_id:)`. +- Commit boundary: `commit_blocker(chat_id:)` returns + `:draft_expired` / `:project_missing` / `:text_missing` / nil, and + `commit_snapshot(chat_id:)` returns a frozen `CommitSnapshot` + (project, text, frozen attachment tuples) so `Supervisor#execute_idea_commit` + and `idea_body_override` never depend on the live mutable Draft. + +Router, CallbackHandlers, and Supervisor now delegate all of these decisions; +`Supervisor#ensure_voice_draft` / `#clear_voice_draft` remain as thin +delegators (they are test seams). No behavior change intended; regression +tests cover each predicate, the reuse/reject semantics, commit blockers, and +the frozen snapshot's independence from later live-draft mutations. + +Validation: `bin/test --changed` (unit bot suites + integration bot scenarios +including s6 voice idea) all passing. \ No newline at end of file diff --git a/wiki/log.d/20260923-fix-guardrail-permission-change.md b/wiki/log.d/20260923-fix-guardrail-permission-change.md new file mode 100644 index 0000000000..b4d421f0a5 --- /dev/null +++ b/wiki/log.d/20260923-fix-guardrail-permission-change.md @@ -0,0 +1,15 @@ +--- +title: Attribute and narrow the fix-guardrail permission_change rule +date: 2026-09-23 +--- + +The post-fix guardrail now seeds each finding's file from the `diff --git` +header, so `permission_change` findings name the file instead of rendering as +`permission_change: :?:`. A new executable under a test, spec, or fixture +directory no longer trips the rule. Previously a fix pass that added the +E2E stub `web/test/e2e/support/codex` parked a review with zero escalations in +`REVIEW_WAITING reason=fix_guardrail`, which surfaced as "Needs review +decision". Mode flips on existing files and new executables elsewhere still +trip. + +See [[stages/review]]. diff --git a/wiki/log.d/20260929-reviewer-merge-base-diff.md b/wiki/log.d/20260929-reviewer-merge-base-diff.md new file mode 100644 index 0000000000..74a5939ee9 --- /dev/null +++ b/wiki/log.d/20260929-reviewer-merge-base-diff.md @@ -0,0 +1,6 @@ +--- +date: 2026-09-29 +summary: Reviewer prompts now compare the task branch with the default branch merge-base. +--- + +All branch-based review templates use `git diff ...HEAD` rather than the two-dot tip range. This prevents reviewer prompts from presenting inverse changes already introduced on the default branch as if they belonged to the task branch. Regression coverage checks every applicable template and the rendered reviewer prompt. diff --git a/wiki/log.d/20261002-deep-freeze-idea-draft-commit-snapshot-leaves.md b/wiki/log.d/20261002-deep-freeze-idea-draft-commit-snapshot-leaves.md new file mode 100644 index 0000000000..5ca1880216 --- /dev/null +++ b/wiki/log.d/20261002-deep-freeze-idea-draft-commit-snapshot-leaves.md @@ -0,0 +1,29 @@ +--- +title: Deep-freeze IdeaDraftStore commit_snapshot String leaves +date: 2026-10-02 +tags: [bot, idea-draft, encapsulation, patrol, fix] +--- + +## Deep-freeze IdeaDraftStore commit_snapshot String leaves + +Patrol-fix review feedback (generation 1, task +`make-one-internal-idea-draft-aggregate-authorita-b22bf56d54f7`) identified a P2 +defect in the commit boundary added by the encapsulation refactor: +`commit_snapshot` froze the `CommitSnapshot` struct, the attachments array, and +each attachment hash, but the String leaves inside them (`project`, `text`, +`staging_path`, `dest_name`, `ext`) were assigned by reference and left +unfrozen. Freeze does not deep-freeze, so the "independent execution view" +still aliased the live mutable Draft: an in-place edit on the live draft +(`draft.text << ...`, `draft.project.replace(...)`, or mutating an attachment +value) rewrote what had already been handed to `Supervisor#execute_idea_commit`. + +### Change + +`IdeaDraftStore#commit_snapshot` now duplicates and freezes every String leaf +via a private `frozen_string` helper (nil passes through for absent text / +project), so the snapshot is an independently frozen view, not a shallow one. +Regression tests cover in-place live-draft mutation after snapshotting and nil +text/project passthrough. + +Validation: `bin/test --changed` (unit bot suites + integration bot scenarios +including s6 voice idea) all passing. diff --git a/wiki/log.d/20261009T013000Z-hive-eval-rejected-rakeopt-stale-report.md b/wiki/log.d/20261009T013000Z-hive-eval-rejected-rakeopt-stale-report.md new file mode 100644 index 0000000000..3d1fa4acfc --- /dev/null +++ b/wiki/log.d/20261009T013000Z-hive-eval-rejected-rakeopt-stale-report.md @@ -0,0 +1,5 @@ +## [2026-10-09T01:30:00Z] testing - rejected inherited RAKEOPT invalidates the stale default report + +**Action:** Updated [[testing]] after fixing `bin/hive-eval`'s inherited-RAKEOPT guard. The guard previously exited `1` before the shared stale-report removal point, so a previously valid `tmp/hive-eval-report.json` survived every rejection (`--dry-run`, `-n`, and conservative malformed-value rejections), letting downstream readers mistake old JSON for the failed invocation's output. The guard now clears the default report before exiting, matching the other pre-run guards; caller-provided report paths remain untouched until full validation. + +**Tests:** Added `test_cli_rejects_inherited_rake_dry_run_clears_stale_default_report` in `test/eval/support/reporter_test.rb`, wrapped in the `preserve_path` helper so the hardcoded default report location is restored for a developer's checkout after the run. \ No newline at end of file diff --git a/wiki/log.d/20261012T000000-probe-credential-keys-cannot-restore-scrubbed-runtime-keys.md b/wiki/log.d/20261012T000000-probe-credential-keys-cannot-restore-scrubbed-runtime-keys.md new file mode 100644 index 0000000000..e1060d4c69 --- /dev/null +++ b/wiki/log.d/20261012T000000-probe-credential-keys-cannot-restore-scrubbed-runtime-keys.md @@ -0,0 +1,19 @@ +# 2026-10-12 — Probe credential keys cannot restore scrubbed runtime keys + +- The generation-2 overlay fix closed `request.environment` as an overlay + channel, but `child_environment` still copied + `request.credential_environment_keys` from the ambient environment after + the Ruby/Bundler denylist scrub. A probe request listing `RUBYOPT` (or + `GEM_HOME`, `GEM_PATH`, `RUBYLIB`, `BUNDLE_*`) as a credential key + restored the poisoned ambient value into the probe child, re-breaking + Ruby-based executable overrides and launcher shims. +- `ProbeRequest` and `OpenCodePreparationRequest` now reject credential + environment keys in the new `SCRUBBED_RUNTIME_ENVIRONMENT_KEYS` constant + with `ArgumentError`, so neither constructor can select a scrubbed + runtime injection key back into a child environment. +- Defense in depth: the probe `child_environment` now applies the denylist + scrub last, after credential selection, executable-override selection, + and the overlay copy, so no caller-controlled channel can reintroduce a + scrubbed key even if a request bypasses constructor validation. +- Regression coverage lives in + `components/agent-cli-runtime/test/opencode_preparation_test.rb`. diff --git a/wiki/log.d/20261012T0420Z-probe-request-environment-restricted-to-overlay-keys.md b/wiki/log.d/20261012T0420Z-probe-request-environment-restricted-to-overlay-keys.md new file mode 100644 index 0000000000..2970fb79b8 --- /dev/null +++ b/wiki/log.d/20261012T0420Z-probe-request-environment-restricted-to-overlay-keys.md @@ -0,0 +1,19 @@ +# 2026-10-12 — Probe request environments are restricted to overlay keys + +- The OpenCode probe's child environment was built with + `selected.merge(request.environment)` after the denylist cleanup, so a + caller-supplied `RUBYOPT` (or `GEM_HOME`, `GEM_PATH`, `RUBYLIB`, + `BUNDLE_*`) key survived into the probe child and broke Ruby-based + executable overrides or launcher shims that the cleanup was supposed to + protect. +- `ProbeRequest` now validates its `environment:` keys against + `OPENCODE_OVERLAY_ENVIRONMENT_KEYS` and raises `ArgumentError` for anything + else. This is a public contract change: constructing a probe request with + non-overlay environment keys that previously passed silently now fails + fast. +- As defense in depth, the probe child environment copies only + overlay-contract keys from the request instead of merging the whole + request environment, so the Ruby/Bundler injection scrub stays final even + if a request bypasses validation. +- Regression coverage lives in + `components/agent-cli-runtime/test/opencode_preparation_test.rb`. diff --git a/wiki/modules/agent_cli_runtime.md b/wiki/modules/agent_cli_runtime.md index 871c2b86d1..45fff1a410 100644 --- a/wiki/modules/agent_cli_runtime.md +++ b/wiki/modules/agent_cli_runtime.md @@ -89,7 +89,19 @@ requested variant while remote model fetching and ambient project configuration are disabled. The probe accepts the caller's already-resolved executable on `ProbeRequest.executable` and otherwise falls back to declared environment overrides; either way it validates and reports exactly the -executable the compiled invocation runs. An exact route declared by the selected overlay is +executable the compiled invocation runs. ProbeRequest environment overrides are +restricted to the OpenCode overlay keys (`OPENCODE_OVERLAY_ENVIRONMENT_KEYS`); +any other key is rejected with `ArgumentError`. The probe child environment +also copies only overlay-contract keys into the scrubbed base rather than +blanket-merging the request environment, and applies the denylist scrub last +after every caller-controlled channel, so Ruby and Bundler injection +variables such as `RUBYOPT` stay removed even when a request is constructed +outside the validated overlay contract. Credential keys are bound by the +same rule: `ProbeRequest` and `OpenCodePreparationRequest` reject any +credential key in `SCRUBBED_RUNTIME_ENVIRONMENT_KEYS` (`RUBYOPT`, `RUBYLIB`, +`GEM_HOME`, `GEM_PATH`, `BUNDLE_BIN_PATH`, `BUNDLE_GEMFILE`) with +`ArgumentError`, so a poisoned ambient value cannot be selected back into a +scrubbed child. An exact route declared by the selected overlay is authoritative and skips the large verbose CLI inventory; an undeclared route must still exist in that bounded local inventory. Generic `probe(profile)` and `prepare!(profile)` remain compatible for legacy profiles. OpenCode's ordinary diff --git a/wiki/modules/bot.md b/wiki/modules/bot.md index 2822ee7342..fb44d0a766 100644 --- a/wiki/modules/bot.md +++ b/wiki/modules/bot.md @@ -27,7 +27,7 @@ answering is now deterministic Q-by-Q.) | `PairingStore` | `lib/hive/bot/pairing_store.rb` | File-backed pending Telegram pairing requests under `/.bot.pairings.json`. Codes are 8-character `A-Z`, chat-id bound, stable while pending, and expire after 24 hours. Cross-process bot/CLI writes use a sidecar flock plus [[modules/atomic_file]] replacement. | | `PairingApprovalQueue` | `lib/hive/bot/pairing_approval_queue.rb` | Owner-only approval notice queue under `/pairing_approvals/`. The `hive pairing approve telegram ` CLI writes notices through [[modules/atomic_file]] so the running bot, which owns the Telegram token, can proactively DM approved chats. | | `Handlers::*` | `lib/hive/bot/handlers/` | Slash command, callback, and free-text logic returning descriptors; no direct Telegram I/O. `SlashHandlers#start` is a pure reply descriptor with a "Connected" welcome plus `/status`, `/idea`, and `/help` next steps. | -| `IdeaDraftStore` | `lib/hive/bot/idea_draft_store.rb` | In-memory per-chat `/idea` draft state with TTL, project/text/attachment metadata, voice-origin and transcript-confirm phases, monotonic attachment counters, temp staging-dir allocation, and cleanup on clear/prune. | +| `IdeaDraftStore` | `lib/hive/bot/idea_draft_store.rb` | In-memory per-chat `/idea` draft state with TTL, project/text/attachment metadata, voice-origin and transcript-confirm phases, monotonic attachment counters, temp staging-dir allocation, and cleanup on clear/prune. It is also the single authority for compound draft-state decisions: routing/callback predicates (`awaiting_transcript_confirmation?`, `non_voice_draft?`, `voice_draft?`, `awaiting_text_draft?`, `transcript_only_voice_draft?`), the voice reuse/reject/clear transitions (`ensure_voice_draft`, `clear_voice_draft`), and the commit gate plus frozen `commit_snapshot` execution view — Router, CallbackHandlers, and Supervisor ask these instead of re-deriving phase/origin/attachment combinations from raw Draft fields. | | `IdeaAttachmentPolicy` | `lib/hive/bot/idea_attachment_policy.rb` | Pure classifier for Telegram photo/document attachments. Allows jpg/jpeg/png/webp/gif/pdf/txt/md/docx, enforces count/byte caps, and normalizes extensions through `Hive::Tui::ComposerStaging`. | | `Transcriber` | `lib/hive/bot/transcriber.rb` | OpenAI-compatible audio transcription client for Telegram voice notes used by idea capture and audio answers. Posts `multipart/form-data` to `bot.transcription.endpoint` with `model`, retries transient failures, maps empty/high no-speech-prob results to `:no_speech`, applies `supported_languages`, and logs failures through the bot logger. | | `StatusWatcher` | `lib/hive/bot/status_watcher.rb` | Reads Hive's hidden internal task graph, validates the envelope, and returns typed task rows carrying slug/id/display_name/`pr_url`/`auto_residue` plus project-level legacy-stage warnings. | @@ -35,8 +35,8 @@ answering is now deterministic Q-by-Q.) | `AlertStore` | `lib/hive/bot/alert_store.rb` | JSON sidecar for alert fingerprints, first-seen timestamps, reminder timestamps, and row snapshots. Corrupt files are renamed aside so the bot keeps running. | | `RowActions` | `lib/hive/bot/row_actions.rb` | Canonical pure resolver from a status row to Telegram chat action descriptors. Both push notifications and `/status` buttons consume this resolver so needs-input rows cannot drift between surfaces. | | `NotificationBuilders` | `lib/hive/bot/notification_builders.rb` | Renders notification text and inline keyboards from `RowActions` descriptors. Every `ERROR` / `REVIEW_ERROR` recovery alert exposes guarded Autofix even when no diagnostic suggested action was recorded; `EXECUTE_STALE` remains manual-only. Rows whose live action is `agent_running` or `archived` suppress stale recovery alerts, but a non-null `auto_residue` summary still produces a keyboard-free FYI naming the committed paths. The builder logs stale-marker contradictions as `notification_skipped_live_agent`. `details_reply(row)` is the shared Show-details renderer for inline callbacks, `/details`, and status row details: it always includes the row summary and next-step hint, appends cached diagnostic summary/detail when present, and caps replies to Telegram's 4096-character limit. All three render sites wrap the call in a soft-degrade rescue (`render_details_reply` in the handlers, an inline rescue in `Supervisor#render_details`): a render-time fault logs `details_render_failed` and replies "Status lookup failed — try again in a moment." rather than escaping past the already-ack'd callback / poll loop and leaving the operator with no reply. The slash `resolve_status_row` degrade now also logs `status_lookup_failed`. The `ready_for_review` approval push appends a clickable PR link when `row.pr_url` is valid. Fingerprints include project, slug, stage, marker, marker attrs, and `auto_residue`, deliberately excluding `pr_url` and rendered text. | -| `BrainstormParser` | `lib/hive/bot/brainstorm_parser.rb` | **Back-compat alias for `Hive::BrainstormParser`** (`lib/hive/brainstorm_parser.rb`) — the pure parser moved to the top-level namespace so the daemon can share it for the answers-pending gate (see [[modules/daemon]] §"Brainstorm answers-pending gate"). Parses one-based `## Round`, `### Q.`, and `### A.` blocks. **Lenient A-header**: accepts the first A-section under each Q as that Q's answer slot regardless of A-number, so a brainstorm-agent off-by-one (e.g. `### A2.` after `### Q1.`) doesn't strand the operator. Stage markers delimit blocks but are excluded from question text/fingerprints. `parse` is total — invalid UTF-8 and a torn concurrent read degrade rather than raise — and reverses only the writer's explicit v1 structural-line encoding; legacy unmarked answers remain literal. Fingerprints use NFC rather than compatibility-folding NFKC. Also exposes the canonical answer heading helper used by the writer. | -| `BrainstormAnswerWriter` | `lib/hive/bot/brainstorm_answer_writer.rb` | Locked, first-write-wins insertion into the next answer block, with atomic rewrite. Slot location is **Q-context-aware**: walks forward from the parser-identified target Q's line, returning the first empty A-section before the next block boundary. When Q{n} is present and unanswered but has **no `### A` slot at all**, the writer **creates** the slot at the end of the Q-block and writes the answer there. The writer scrubs invalid UTF-8, normalizes lone-CR input before ordinal mapping, reversibly neutralizes structural answer lines under an explicit v1 answer header, and takes the existing marker sidecar lock around read-plus-atomic-replace so marker and answer writers cannot clobber each other. Identity-bound writes use bounded, creation-disabled marker locking; the unreachable exact-writer `:answer_slot_missing` fallback is removed. Giving up on the lock emits `:answer_lock_contention` carrying the holder metadata from the rescued `Hive::ConcurrentRunError`; the holder is captured before the deadline check, so a `try_append` slower than the whole retry budget still names the culprit instead of reporting `holder: nil`. | +| `BrainstormParser` | `lib/hive/bot/brainstorm_parser.rb` | **Back-compat alias for `Hive::BrainstormParser`** (`lib/hive/brainstorm_parser.rb`) — the pure parser moved to the top-level namespace so the daemon can share it for the answers-pending gate (see [[modules/daemon]] §"Brainstorm answers-pending gate"). Parses one-based `## Round`, `### Q.`, and `### A.` blocks. **Lenient A-header**: accepts the first A-section under each Q as that Q's answer slot regardless of A-number, so a brainstorm-agent off-by-one (e.g. `### A2.` after `### Q1.`) doesn't strand the operator. Stage markers delimit blocks but are excluded from question text/fingerprints. `parse` is total — invalid UTF-8 and a torn concurrent read degrade rather than raise — and reverses only the writer's explicit v1 structural-line encoding; legacy unmarked answers remain literal. Fingerprints use NFC rather than compatibility-folding NFKC. Also exposes the canonical answer heading helper used by the writer. **Location source of truth**: each `Question` carries `question_line_index`, `block_end_index` (first Q/Round/marker boundary at-or-after the header, or EOF), and `answer_line_index`, all indexed against `document_lines(text)` — the parser's newline-normalized line array. Mutation callers (the answer writer) anchor on these fields instead of re-scanning raw lines, so parser and writer share one definition of a document line and one boundary rule. | +| `BrainstormAnswerWriter` | `lib/hive/bot/brainstorm_answer_writer.rb` | Locked, first-write-wins insertion into the next answer block, with atomic rewrite. Slot location is anchored on the parser's `Question` location fields: the parser-identified unanswered target supplies the Q header line, the A-header line, and the block-end index, so the writer fills the empty A-section before `block_end_index` or inserts a created slot exactly at it — no writer-side re-scan of raw lines and no separate boundary rule (line surgery runs on the parser's `document_lines` array re-terminated with `\n`, and the written document keeps the pre-write line-ending style: CRLF stays CRLF). When Q{n} is present and unanswered but has **no `### A` slot at all**, the writer **creates** the slot at the end of the Q-block and writes the answer there. The writer scrubs invalid UTF-8, normalizes lone-CR documents through the same normalization the parser indexes against, reversibly neutralizes structural answer lines under an explicit v1 answer header, and takes the existing marker sidecar lock around read-plus-atomic-replace so marker and answer writers cannot clobber each other. Identity-bound writes use bounded, creation-disabled marker locking; the unreachable exact-writer `:answer_slot_missing` fallback is removed. Giving up on the lock emits `:answer_lock_contention` carrying the holder metadata from the rescued `Hive::ConcurrentRunError`; the holder is captured before the deadline check, so a `try_append` slower than the whole retry budget still names the culprit instead of reporting `holder: nil`. | | `ConversationStore` | `lib/hive/bot/conversation_store.rb` | In-memory per-chat active answer state with TTL. No sidecar file; `brainstorm.md` stays canonical. State carries `chat_id`, `project`, `slug`, displayed `question_n`, the opaque identity `binding`, `mode`, and `updated_at`; the retired Codex draft-assist fields (`history`, `draft`, `awaiting_confirm`) and `pending_confirm_count` API are gone. **A chat holds at most one active conversation: `start` deletes any other entry for the same `chat_id` before inserting**, so starting `/answer task-b` while `task-a` is open supersedes it and the slug-less lookups used by the free-text path (`FreeTextHandler`, `Router#answer_context`, slash `/done`) always resolve to the most recently started conversation instead of a stale first match. Telegram writes pass the binding through the router to `Hive::Commands::Answer`, so same-number questions from replacement rounds fail stale rather than receiving an old reply. All `@states` reads/writes are mutex-guarded because the Telegram poll thread mutates conversations while the status-poll thread calls `active_for_slug?` and prunes. `active_for_slug?(slug, project: nil)` is prune-aware and returns true when any chat has a non-expired answer conversation for that slug. Project scoping is lenient: two fully resolved different projects do not cross-suppress, but an unscoped/project-less side still suppresses to avoid reintroducing duplicate waiting alerts. `NotificationDispatcher` uses this to avoid duplicate proactive waiting pushes while the operator is already answering. | | `ChildSupervisor` | `lib/hive/bot/child_supervisor.rb` | Spawns child `hive ...` commands with `pgroup: true`, writes per-dispatch logs, reaps with `Process.wait2(-1, WNOHANG)`. After plan 2026-05-28-002 the bot only reaches this path for **non-queue-routable** verbs (`hive status --diagnose --write --force` for Refresh diagnostic, `hive new` for the idea picker, `hive approve` for the /approve slash command, `hive accept-finding` / `hive reject-finding` for findings replies). State-mutating workflow verbs are written to the daemon's dispatch-request queue instead — see `DispatchRequestWriter` below. | | `DispatchRequestWriter` | `lib/hive/bot/dispatch_request_writer.rb` | Client of the shared runtime-control-plane dispatch repository. It validates argv before inserting a durable request row, then attempts immediate non-interactive admission; accepted attempts bind the same request row, capacity or unavailable-local cases remain queued, and unexpected failures remove only a still-unclaimed row. Recoverable-marker requests delegate to `Hive::Recovery::API`. | diff --git a/wiki/modules/reviewers.md b/wiki/modules/reviewers.md index 7688f346cb..e2e205d322 100644 --- a/wiki/modules/reviewers.md +++ b/wiki/modules/reviewers.md @@ -56,6 +56,8 @@ Claude agent reviewers take the same `run!` / `spawn_agent` path as every other `status_mode: :output_file_exists` is critical: reviewer spawns own a per-pass output file, not the task marker — the orchestrator's `REVIEW_WORKING` marker must persist across each reviewer's spawn (per ADR-021). +All branch-based reviewer prompts scope their work with `git diff ...HEAD`. The three-dot range compares `HEAD` with the merge-base, so a reviewer sees only changes introduced by the task branch; the former two-dot tip comparison could show inverse changes that had already landed on the default branch. + ## `AgentSupport::Codex::Reviewer` Native-`codex review` adapter (added 2026-06-10). The **patrol-default** reviewer: one cheap, tuned, read-only `codex review` pass instead of the multi-persona `ce-code-review` fan-out (6–18 subagents). `run!`: diff --git a/wiki/stages/review.md b/wiki/stages/review.md index 5dc4cf65b7..006a109805 100644 --- a/wiki/stages/review.md +++ b/wiki/stages/review.md @@ -186,7 +186,7 @@ After the fix agent returns, `Hive::Stages::Review::FixGuardrail.run!` (ADR-020 - `ci_workflow_edit` — `.github/workflows/`, gitlab-ci, circleci, Jenkinsfile, bitbucket-pipelines, azure-pipelines, travis - `secrets_pattern_match` — retains the descriptor key `detector: :secret_patterns` for config compatibility, but delegates detection to bundled Betterleaks. Added lines are batched by filename; upstream source-aware rules own classification. Findings contain redacted snippets and exact secret hashes, preserving precise waiver matching without exposing credentials. - `dotenv_edit` — `.env`, `.env.` (e.g., `.env.local`, `.env.production`, `.env.test`, `.env.staging`), `secrets.yml`, `credentials.yml(.enc)`, `.npmrc`, `.pypirc`. Template suffixes are deliberately **excluded** so committed templates do not trip the guardrail: `.env.example`, `.env.sample`, `.env.template`, `.env.dist`, `.env.tmpl`, `.env.default`, `.env.defaults`. Projects that genuinely keep secrets in `.env.example` can re-add strict matching via `review.fix.guardrail.patterns_override` (custom `dotenv_template_edit` pattern). -- `permission_change` — `new mode 100755` raw-diff-header +- `permission_change` — executable-bit raw diff header (`new file mode 100755`, `new mode 100755`). The finding's file comes from the `diff --git` header, because mode lines precede the `---`/`+++` pair and a pure chmod emits no pair at all (previously every such finding rendered as `permission_change: :?:`). A **new** executable under a `test/`, `tests/`, `spec/`, `specs/`, `__tests__/`, `testdata/`, or `fixture(s)/` directory is exempt (`exempt_new_file_paths`) — fake CLI stubs such as `web/test/e2e/support/codex` must be executable to shadow the real binary on PATH, and tripping on them parked clean reviews in `REVIEW_WAITING` with zero escalations. A mode flip on an existing file, or a new executable anywhere else, still trips. Per-project override via `review.fix.guardrail.patterns_override`: `false` to disable a default; Hash to add a custom (must include `regex`). Replacement is total: a Hash under any name — including `secrets_pattern_match` — installs a plain regex detector (`detector: :regex`) and never inherits the Betterleaks dispatch. Scan raises on any descriptor whose `detector:` is neither `:regex` nor `:secret_patterns`. Tripped → `REVIEW_WAITING reason=fix_guardrail matches=N head= pass=NN` and `reviews/fix-guardrail-NN.md` written. The `head=` attribute records the worktree HEAD at the moment the guardrail tripped; the approval-on-resume path enforces it. Because decoded paths and matched snippets may contain literal control bytes (a C-quoted rename target like `".github/workflows/de\nploy.yml"` decodes to a real newline), the report writer escapes every control byte at the report boundary (`escape_control_bytes`): one Match always renders as exactly one unchecked checkbox line, so a crafted path fragment such as `\n- [x] ` can neither forge a pre-checked box nor drift the checkbox count away from the marker's recorded matches and deadlock the approval round-trip. diff --git a/wiki/templates.md b/wiki/templates.md index 8734e95742..335a9acba5 100644 --- a/wiki/templates.md +++ b/wiki/templates.md @@ -133,7 +133,7 @@ User-supplied template paths under `<.hive-state>/templates/` are resolved via ` | `browser_test_prompt.md.erb` | `Stages::Review::BrowserTest#run_attempt` (Phase 5) | `project_name`, `worktree_path`, `task_folder`, `pass`, `attempt`, `max_attempts`, `result_path`, `skill_invocation`, `user_supplied_tag` | | `triage_courageous.md.erb` | `Stages::Review::Triage` (Phase 3 default bias) | `project_name`, `worktree_path`, `task_folder`, `pass`, `reviewer_files`, `reviewer_contents`, `escalations_path`, `user_supplied_tag` | | `triage_safetyist.md.erb` | `Stages::Review::Triage` (opt-in bias preset) | same as `triage_courageous.md.erb` | -| `reviewer_claude_ce_code_review.md.erb` | `Reviewers::Agent#render_prompt` (Phase 2) | `project_name`, `worktree_path`, `task_folder`, `default_branch`, `pass`, `output_path`, `skill_invocation`, `user_supplied_tag` | +| `reviewer_claude_ce_code_review.md.erb` | `Reviewers::Agent#render_prompt` (Phase 2); reviews `git diff ...HEAD` from the merge-base | `project_name`, `worktree_path`, `task_folder`, `default_branch`, `pass`, `output_path`, `skill_invocation`, `user_supplied_tag` | | `reviewer_codex_ce_code_review.md.erb` | `Reviewers::Agent#render_prompt` (Phase 2) | same as above | | `reviewer_pr_review_toolkit.md.erb` | `Reviewers::Agent#render_prompt` (Phase 2) | same as above | | `finalize_prompt.md.erb` | `Stages::Finalize.run!` | `project_name`, `task_folder`, `worktree_path`, `slug`, `pr_url`, `plan_text`, `reviews_summary`, `user_supplied_tag` | diff --git a/wiki/testing.md b/wiki/testing.md index 1727377333..a0d0b17a84 100644 --- a/wiki/testing.md +++ b/wiki/testing.md @@ -1227,7 +1227,7 @@ bin/hive-eval --scenario s1_status --no-judge --report /tmp/hive-eval.json `test/eval/support/` provides an in-process fake Telegram transport, a programmable status watcher, child-supervisor and dispatch-request captures, a scenario DSL, typed-reason contract assertions, scripted/Codex personas, and a Codex prose judge. Scenario files live under `test/eval/scenarios/` and drive the real `Hive::Bot::Supervisor#process_update` / `#status_tick` entrypoints without changing production bot behavior. Queue-routable bot verbs are captured through the fake `DispatchRequestWriter`, and `Harness#dispatched_commands` lets scenarios assert command intent across both queued and child-spawned dispatch paths. -`bin/hive-eval` is a checkout-local OptionParser wrapper over `rake test:eval`. It accepts only `--scenario NAME`, `--report PATH`, and `--no-judge`; relative report and scenario-root paths resolve against the repository root, independent of the caller's cwd. Invalid options, missing option values, stray positional arguments, missing scenarios, and unsafe scenario names exit `64` before eval runs. Parser-error cleanup may clear only the default `tmp/hive-eval-report.json`; caller-provided report paths are preserved on usage and validation exits. After positional and scenario validation pass, the runner removes the selected report path immediately before invoking `rake test:eval` so valid runs cannot read stale JSON from a previous run. Unexpected positional-argument errors label the count as `argument` or `arguments`; `--scenario` resolves a basename under the scenario root after stripping an optional `_test` suffix. Slash and backslash path separators get a distinct usage error, and separator-free names still must match `[A-Za-z0-9_-]+` before being joined under `test/eval/scenarios/` or `HIVE_EVAL_SCENARIO_ROOT`. All-scenario runs clear inherited `TEST` and set `HIVE_EVAL_SCENARIOS_ONLY=1` so ambient test selection cannot execute support or unit files. The wrapper also owns `HIVE_EVAL_NO_JUDGE`: it passes `1` only when `--no-judge` is present and otherwise clears any inherited value so caller environment cannot silently disable judges. A missing/unspawnable Bundler command exits `127` with a direct diagnostic. Tests may point `HIVE_EVAL_SCENARIO_ROOT` at a temporary scenario directory for throwaway fixtures. +`bin/hive-eval` is a checkout-local OptionParser wrapper over `rake test:eval`. It accepts only `--scenario NAME`, `--report PATH`, and `--no-judge`; relative report and scenario-root paths resolve against the repository root, independent of the caller's cwd. Invalid options, missing option values, stray positional arguments, missing scenarios, and unsafe scenario names exit `64` before eval runs. Parser-error cleanup may clear only the default `tmp/hive-eval-report.json`; caller-provided report paths are preserved on usage and validation exits. After positional and scenario validation pass, the runner removes the selected report path immediately before invoking `rake test:eval` so valid runs cannot read stale JSON from a previous run. A rejected inherited `RAKEOPT` (dry-run spellings or unparseable values) also clears the default report before its exit `1`, so the pre-run guards cannot strand stale JSON for downstream readers; caller-provided report paths remain untouched until full validation. Unexpected positional-argument errors label the count as `argument` or `arguments`; `--scenario` resolves a basename under the scenario root after stripping an optional `_test` suffix. Slash and backslash path separators get a distinct usage error, and separator-free names still must match `[A-Za-z0-9_-]+` before being joined under `test/eval/scenarios/` or `HIVE_EVAL_SCENARIO_ROOT`. All-scenario runs clear inherited `TEST` and set `HIVE_EVAL_SCENARIOS_ONLY=1` so ambient test selection cannot execute support or unit files. The wrapper also owns `HIVE_EVAL_NO_JUDGE`: it passes `1` only when `--no-judge` is present and otherwise clears any inherited value so caller environment cannot silently disable judges. A missing/unspawnable Bundler command exits `127` with a direct diagnostic. Tests may point `HIVE_EVAL_SCENARIO_ROOT` at a temporary scenario directory for throwaway fixtures. Successful eval runs write a `hive-eval-report` JSON document with per-scenario assertions/messages/log events, and scenario failures make the wrapper exit non-zero. `ReportStore.write!` neutralizes any symlink or hard link at the report path before writing (unlink, then open with `O_CREAT|O_EXCL|O_NOFOLLOW`) so the report always lands on a fresh regular file and a `--report` path pointing at a link never clobbers the link's target. `--no-judge` is the explicit structural-only mode; otherwise Codex judge/persona calls are real subprocess calls. Scenario `s3_noise` is now a passing daemon-enabled noise regression: ready-to-action rows should not become proactive Telegram alerts when the daemon owns dispatch, and the scenario still asserts no duplicate messages plus the proactive allowlist (`agent_blocked_question`, `fatal_error`). Reporter failure-path coverage no longer relies on a production scenario staying red; `test/eval/support/reporter_test.rb` creates a tmpdir-scoped intentional-failure scenario through `HIVE_EVAL_SCENARIO_ROOT`.