Repository navigation
fix: batch Patrol fixes and the review guardrail attribution fix - #1535
Merged
Merged
Conversation
… stubs The post-fix guardrail cleared the current file on each `diff --git` line, but mode lines come before the ---/+++ pair (and a pure chmod has no pair), so every permission_change finding rendered as `:?:`. Seed the path from the diff --git header instead. With the path known, a new executable under a test/spec/fixture directory no longer trips permission_change. 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. Mode flips on existing files and new executables elsewhere still trip. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ysitter/pr-1468 # Conflicts: # test/e2e/lib/incident_budget.rb # test/e2e/lib/incident_budget_test.rb # wiki/gaps.md # wiki/testing.md
The shared brainstorm parser returned only logical question data
(round, number, text, answer), so BrainstormAnswerWriter had to
reconstruct 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 meant every future format change was a synchronized
cross-boundary edit, and the raw-line re-scan indexed a different line
array than the parser used (a lone-CR file yields more parser lines than
content.lines).
Centralize document location in Hive::BrainstormParser:
- Question now 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 the parser's
newline-normalized line array, now exposed as document_lines.
- The writer anchors directly on those fields: it selects the
parser-identified unanswered target, fills the empty A-slot at
answer_line_index before block_end_index, or inserts a new slot at
block_end_index - no re-scan, no separate boundary rule.
- Writer line surgery runs on the parser's own line 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). The lone-CR write path now normalizes the whole document the
same way the parser does.
The removed re-scan (target_question_line_index) was the last place a
writer-side positional guess could diverge from the parser, which is what
previously let a round boundary be crossed when routing an answer.
EOF-final questions (no trailing newline) keep their terminator, and
lone-CR documents now land answers instead of dead-ending in
:answer_slot_missing.
Branch-only reviewer templates told the reviewer to run `git diff <default_branch>..HEAD` — the two-dot tip-to-tip range. When the default branch advanced after the feature branch was cut, that range also shows the inverse of the new default-branch commits, so reviewers saw unrelated "changes" the branch never introduced and could file phantom findings. The codex native reviewer already used the correct three-dot merge-base range. All reviewer templates (Grok, pr-review-toolkit, Claude, Codex CE, and the legacy execute-review prompt) now render `git diff <default_branch>...HEAD` so every reviewer sees only the changes the branch itself added. A new template test pins the merge-base notation across all reviewer templates.
(cherry picked from commit 17ba8ca)
IdeaDraftStore declared the phase/origin contract as its own, yet Router, CallbackHandlers, and Supervisor each re-derived compound interpretations from the live mutable Draft Struct: transcript-confirm routing, the voice-during-draft short-circuit, immediate-commit-vs-file-collection, voice reuse/reject/clear, and commit validation that consumed raw attachment tuples. The store now owns those decisions behind an explicit API: routing and callback predicates (awaiting_transcript_confirmation?, non_voice_draft?, voice_draft?, awaiting_text_draft?, transcript_only_voice_draft?), the ensure_voice_draft/clear_voice_draft transitions, and a commit boundary of commit_blocker plus a frozen commit_snapshot execution view. Consumers delegate to them, so execution no longer depends on the store's mutable representation, and the phase/origin invariants have a single authority. No behavior change; regression tests cover every predicate, the reuse and reject semantics, commit blockers, and snapshot immutability.
Review of the encapsulation fix found the frozen commit snapshot still aliased the live Draft's mutable Strings: freeze does not deep-freeze, so project, text, and attachment values (staging_path, dest_name, ext) were shared with the live draft and in-place edits on it (<<, replace) rewrote what had already been handed to execute_idea_commit. commit_snapshot now duplicates and freezes every String leaf (nil passes through), making the snapshot a genuinely independent execution view. Regression tests cover in-place live-draft mutation after snapshotting and nil text/project passthrough.
…ected bin/hive-eval's inherited-RAKEOPT guard exited 1 before the shared stale-report removal point, so a previously valid report at tmp/hive-eval-report.json survived every RAKEOPT rejection (--dry-run, -n, or malformed values). Clear the default report on that path, matching the other pre-run guards; custom report paths are still only removed after full validation. The regression is wrapped in the reporter test's preserve_path helper so it restores a developer's pre-existing default report instead of clobbering the hardcoded checkout path, and the hive-eval report lifecycle contract in the wiki is updated with a log fragment.
…bbed runtime keys ProbeRequest accepted arbitrary environment keys, and OpenCode probe child_environment applied the request overlay with Hash#merge after the RUBYOPT/GEM_HOME/etc. denylist cleanup. A caller-supplied denylisted key therefore survived into the child environment and broke Ruby-based executable overrides or launcher shims with injected load paths. Root-cause fix on both sides of the contract: - ProbeRequest now validates that environment keys are OpenCode overlay keys and rejects anything else with ArgumentError. - child_environment no longer blanket-merges the request environment; it copies only overlay-contract keys, so the denylist scrub stays final even if a request is constructed with keys outside the validated overlay contract. Regression coverage: ProbeRequest rejects each denylisted runtime key, and child_environment keeps scrubbed keys nil while overlay keys and ambient safe keys still flow through.
Review rework on the scrubbed-runtime-keys fix required the bounded wiki artifacts: the agent_cli_runtime module page now documents that ProbeRequest rejects environment keys outside OPENCODE_OVERLAY_ENVIRONMENT_KEYS and that the probe child environment copies only overlay-contract keys so the denylist scrub stays final. A wiki/log.d fragment records the contract change for the compiled log.
…ubbed 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/Gem 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 the scrub protects. Root-cause fix on both sides of the contract: - New shared SCRUBBED_RUNTIME_ENVIRONMENT_KEYS constant names the Ruby, Bundler, and RubyGems injection keys the probe scrubs. - ProbeRequest and OpenCodePreparationRequest reject any credential key in that list with ArgumentError, so neither constructor can select a scrubbed key back into a child environment. - Defense in depth: 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: both request constructors reject each scrubbed credential key, and child_environment keeps the scrubbed keys nil while credential values and overlay keys still flow through. Wiki: the agent_cli_runtime module page documents the credential-key contract, and a log fragment records the change.
# Conflicts: # test/e2e/lib/incident_budget_test.rb # wiki/gaps.md
# Conflicts: # test/e2e/lib/incident_budget_test.rb
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combines the remaining ready PRs into one branch so they share a single CI run. Each PR head is merged as-is.
RAKEOPTis rejectedIdeaDraftStore; deep-freezecommit_snapshotstring leavespermission_changefindings and exempt new test stubsThe Patrol fixes (1530, 1525, 1513, 1516, 1461) were validated by Patrol but not reviewed by Hive.
Conflict resolutions
mainalready has from chore(deps): batch dependency bumps, babysitter CI fixes, and 0.8.0 changelog notes #1533.main's test sample andwiki/gaps.mdtext are kept, and two duplicate wiki fragments are dropped.Test plan
test/unit/bot/*,reviewers/agent_test,stages/review/*,templates/*,integration/run_review_test, and all agent-cli-runtime component tests.eval/support/reporter_testandagent_cli_runtime_component_testfail locally exactly as onmain(brakeman 8.1.0 and bundler not installed locally).After merge, the individual PRs get closed with a pointer here.
🤖 Generated with Claude Code