Skip to content

feat(pr-review): controller-mediated, head-bound PR review submission tool - #3118

Merged
zaxbysauce merged 7 commits into
mainfrom
fix/3096-pr-review-submission-tool
Oct 7, 2026
Merged

zaxbysauce merged 7 commits into
mainfrom
fix/3096-pr-review-submission-tool

Conversation

@zaxbysauce

@zaxbysauce zaxbysauce commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #3096

Summary

Adds the missing controller-mediated, head-bound PR review submission tool
(Workstream C PR 1/2, epic #3102). The new architect-only pr_review_submission
tool submits a settled PR-review run to GitHub through the PR Review API
(gh api repos/<o>/<r>/pulls/<n>/reviews --method POST --input, bounded gh
transport) with the review's commit_id pinned to the run's exact
pr_head_sha and inline comments carrying each finding's file:line identity.
A new pure module renderPrReviewSubmissionBody (no fs/network/clock) renders
the severity-grouped body (CRITICAL..LOW, non-empty groups only) with finding
ids, locations, and coverage disclosure; reviewer-behavior constraints hold:
already-posted findings are skipped, repeated finding ids and identical
locations consolidate, inline comments cap at 20 with a disclosed truncation
marker, and an all-posted run refuses as an idempotent no-op.

Authorization is fail-closed and purely additive (no settlement, gate, or
artifact path changes; pr-workflow-gate.ts untouched): refuses while any
PR-workflow gate is active for the session, refuses when the workflow for this
head was aborted at/after the run's settlement time (bounded events-window scan
with an ISO-8601 recency anchor that preserves the abort-and-retry recovery
flow), and refuses unless the trigger-eval receipt and every findings record
bind to the declared head with at least one post_critic settlement record. The
child discovery/validation overlay is unchanged — nothing auto-posts from a
lane. Disclosed limits (crashed-run inference, cross-session invocation,
single-page dedupe fetch, body never re-deduped, abbreviated-SHA commit_id,
bounded abort window) are documented in the release fragment.

Invariant audit

  • 1 (plugin init): not touched — no init-path changes; new module is loaded lazily via the manifest thunk.
  • 2 (runtime portability): not touched — plain Node-compatible TS, no bun: imports, no Bun.* calls (bun run typecheck clean; bundle-portability surfaces untouched).
  • 3 (subprocesses): touched — the only new spawn sites are the gh GET/POST calls through the sanctioned runExternalTool runner (array-form args, explicit cwd, 20 s timeout, 2 MiB/128 KiB caps, kill-tree) with the executable always from resolveGhBinary(); bun run check:bare-spawn passed ("no bare {git, gh, ...} spawn call sites").
  • 4 (.swarm containment): touched — reads/writes stay under .swarm/pr-review/<run_id>/ via validateSwarmPath; payload provenance copy at submission-payload.json; run_id charset excludes traversal; working_directory override resolved by the shared project-root resolver (allowWorkingDirectoryOverride: true, the write_pr_review_artifact precedent).
  • 5 (plan durability): not touched.
  • 6 (test_runner safety): not touched.
  • 7 (test writing): touched — bun:test only, _internals DI seam (no mock.module anywhere in the three new files), canonicalMkdtemp + closeAllProjectDbs cleanup, files 323/434/166 lines under the 500 cap (bun run check:test-file-cap 0 violations; bun run check:test-clock passed — literal ISO fixtures).
  • 8 (session state): not touched — the tool is stateless; no module-level session maps.
  • 9 (guardrails/retry): touched — single bounded call per action with typed failure channels (gh-not-found / timeout / spawn-error / nonzero exit); no retry loops (pinned by the parameterized POST-failure test asserting exactly one POST attempt); no auto-APPROVE (event is REQUEST_CHANGES for CRITICAL/HIGH findings, else COMMENT).
  • 10 (chat/system msg): not touched.
  • 11 (tool registration): touched — full additive set: TOOL_METADATA entry (architect-only, no prWorkflow key → the existing gate classifier fail-closes it during active gates), TOOL_MANIFEST thunk, barrel export, three test files; bun run scripts/check-tool-registration.ts → "139 tools, coherent across metadata, handlers, the plugin object, TOOL_NAMES, AGENT_TOOL_MAP, and the barrel"; all 122 tests/unit/config suites green.
  • 12 (release/cache): touched — docs/releases/pending/3096-pr-review-submission-tool.md fragment added with disclosed limits; no version files touched.

Test plan

All at base f102d09 → head bd79848 (four commits: 2fd76f8 implementation,
ea14fc4 review-round arms, 7f32bef retention-registry registration,
bd79848 registry-doc grammar lockstep — the last two fix the CI quality
gate check:retention; production code unchanged after 2fd76f8):

  • Frozen acceptance checks (independent check-author context, pre-implementation
    checkpoint anchored at issue [Workstream C] PR 1 of 2: Controller-mediated, head-bound PR review submission tool #3096 comment 6011535643, manifest verified
    byte-identical post-implementation): C1-C4 DISCRIMINATING RED→GREEN, C5
    PRESERVING GREEN→GREEN — all five PASS at head (logs in the trace).
  • New suites: bun --smol test tests/unit/tools/pr-review-submission.test.ts
    (12 pass), tests/unit/tools/pr-review-submission-transport.test.ts (11
    pass), tests/unit/pr-review/render-review-body.test.ts (9 pass) — 0 fail.
  • Mutation probes (tier M, remote-write authorization risk trigger): 6/6 bit —
    gate refusal, abort-event check, receipt head equality, post_critic
    requirement, event derivation, commit_id binding each flip their target RED
    and restore GREEN.
  • Gates: bun run typecheck exit 0; biome clean on touched files; bun run scripts/check-tool-registration.ts 139 coherent; bun run check:bare-spawn
    passed; bun run check:test-clock passed; bun run check:test-file-cap 0
    violations; bun run check:invariants all passed; scan-deferred clean;
    bun run drift:check --enforce — the single blocking finding is the
    pre-existing checkout-local WORKFLOW_CHANGED_AFTER_CAPTURE on
    .github/workflows/pr-standards.yml (this diff does not touch that file or
    scripts/required-check-contract.json; no new drift findings).
  • Blast radius: all 122 tests/unit/config/*.test.ts green per-file; per-file
    battery over all 680 tests/unit/tools/*.test.ts: 673 green, 7 files fail
    with counts byte-identical at base in a disposable base worktree
    (consensus-mine, council-attempt, save-plan-profiles,
    write-architecture-supervisor-evidence) — pre-existing host-dependent
    failures in files this PR does not touch.
  • Gate ladder: plan critic 3 rounds → APPROVE; independent implementation
    review round 1 NEEDS_REVISION (unpinned GET timeout/spawn-error arms found)
    → arms added → round 2 APPROVE; final critic APPROVE, every AC mapped to evidence;
    post-CI delta rounds: reviewer Rounds 3/4 (registry doc lockstep) APPROVE
    at bd79848, delta final critic bookkeeping revisions all completed
    (fallback context; pinned critic route had provider-auth failures).

Test User added 2 commits October 6, 2026 03:25
… tool (#3096)

Adds the pr_review_submission architect-only controller tool and a pure
renderPrReviewSubmissionBody renderer module. Submits settled PR-review runs
to GitHub through the bounded gh transport (POST .../reviews) with commit_id
pinned to the run's exact pr_head_sha and file:line inline comments.
Fail-closed authorization: refuses while a PR-workflow gate is active, on an
abort at/after settlement, on receipt/record head mismatches, and without a
post_critic settlement record. Additive registration only (invariant 11);
no settlement, gate, or artifact path changes.
…und 1)

Adds the arms the independent implementation review found unpinned: GET
timeout and spawn-error refusals (previously a fail-open partial fix would
have passed every test), POST timeout/spawn-error/nonzero typed refusals,
the no-session-ID refusal, and the receipt run_id mismatch refusal. Also
discloses the bounded-events-window abort escape in the release fragment.
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Drift check report

Found 2 drift finding(s): 0 error, 0 warning, 2 notice.

required-check-contract (2)

  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not yet required by the captured ruleset
  • 🔵 notice scripts/required-check-contract.json: [RULESET_DIVERGENCE] intended-required context "drift" is not present for every expected event in captured external workflow evidence

Quarantine census

Quarantine census
ledger scripts/ci/quarantined-tests.txt: 1 active
ledger scripts/ci/quarantined-tests-windows.txt: 0 active
ledger scripts/ci/quarantined-tests-macos.txt: 2 active
ledger scripts/ci/quarantined-integration-tests.txt: 0 active
total active: 3
histogram 2026-11-18: 3
first hard-fail date: 2026-12-03
days-to-first-wall: 58
owners: zaxbysauce
trend: +14/-13 over 30d

Test User added 2 commits October 6, 2026 05:14
…artifacts row

The quality job's retention check (issue #2036) flagged the new module as
an unregistered durable writer: submission-payload.json is one provenance
copy per POST attempt under the existing .swarm/pr-review/{run_id}/ stream,
so the module joins that row's writerModules and the path grammar gains the
file name.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The prior-comment dedupe matches a finding id as a raw substring, which can silently drop a genuinely new finding from a remote-write review submission.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

This PR adds a new architect-only controller tool, pr_review_submission (Workstream C PR 1/2 of epic #3102), that closes the last provenance gap in the PR-review pipeline: publishing settled review results back to GitHub. It submits a settled PR-review run through the GitHub PR Review API (POST repos/<o>/<r>/pulls/<n>/reviews via the bounded gh transport), pins commit_id to the run's exact pr_head_sha, and attaches inline comments for findings with file:line evidence. A new pure module renderPrReviewSubmissionBody builds the severity-grouped body and inline-comment plan with no fs/network/clock, so the renderer can be reused by future surfaces (e.g. #3097). The change is additive — it touches no settlement, gate, or artifact path.

Changes:

  • New pr_review_submission tool with a fail-closed authorization ladder (gate-cleared, abort-after-settlement narrowing, trigger-receipt and findings head-binding, post_critic settlement requirement) and a bounded gh GET→POST transport via the _internals DI seam.
  • New pure renderPrReviewSubmissionBody renderer: severity grouping, coverage disclosure, prior-comment dedupe, repeated-finding/location consolidation, and a 20-comment cap with a disclosed truncation marker.
  • Full registration (tool-metadata, manifest, barrel), retention-registry + docs lockstep for submission-payload.json, a release fragment, and three new test suites.
File Description
src/​tools/​pr-review-submission.ts New tool: arg validation, authorization ladder, dedupe fetch, payload write, and POST transport.
src/​pr-review/​render-review-body.ts New pure renderer for the severity-grouped body and inline-comment plan.
src/​tools/​tool-metadata.ts Adds the architect-only pr_review_submission metadata entry.
src/​tools/​manifest.ts Imports and registers the manifest handler thunk.
src/​tools/​index.ts Barrel export for the tool and its executor.
scripts/​retention-registry.data.ts Registers the new submission-payload.json writer and path grammar.
docs/​observability-retention-registry.md Doc-side lockstep for the new artifact in the registry row.
docs/​releases/​pending/​3096-pr-review-submission-tool.md Release fragment documenting the tool and disclosed limits.
tests/​unit/​tools/​pr-review-submission.test.ts Registration, args, and authorization-ladder coverage.
tests/​unit/​tools/​pr-review-submission-transport.test.ts Transport ordering, event mapping, dedupe-fetch, and failure arms.
tests/​unit/​pr-review/​render-review-body.test.ts Renderer grouping, dedupe, consolidation, and cap coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/pr-review/render-review-body.ts Outdated
Comment on lines +116 to +121
const id = finding.finding_id;
if (existingComments.some((comment) => comment.includes(id))) {
if (!seenIds.has(id)) skippedAsPosted.push(id);
seenIds.add(id);
continue;
}
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

PR Review — #3118 feat(pr-review): controller-mediated, head-bound PR review submission tool

Verdict: REQUEST_CHANGES
Risk: HIGH
Base: f102d098 → head: bd798482
Findings reviewed: 37 (8 correctness-state · 7 tests-falsifiability · 5 security-trust · 11 reliability-performance · 6 intent-architecture)
Independent validation: Reviewer pass + critic (mega_reviewer) challenge on actual code at HEAD bd798482.


Critical (HIGH severity) — 2 upheld, 1 downgraded

# Finding File:line Verdict Rationale
1 correctness-state-001 — Historical superseded records are first-wins rendered instead of terminal projection; post_critic DISPROVED rows still publish with explorer evidence, and reviewEventFor scans all records so a superseded HIGH can force REQUEST_CHANGES src/tools/pr-review-submission.ts:519 + src/pr-review/render-review-body.ts:117-131 UPHELD by critic Writer at write-pr-review-artifact.ts:213-218,739-750 explicitly uses latest-wins; submission path does not.
2 correctness-state-002 — deriveCoverage accepts only string unresolvedDimensions while V2 writer emits {dimension,terminalState,reasonKind,...} object records; a settled PARTIAL run silently renders FULL src/tools/pr-review-submission.ts:230-237 (string-only filter) + src/pr-review/completion.ts:594-597,1431-1437 (object writer) UPHELD by critic typeof name === 'string' filters out all V2 disclosures; without receipt degradations, S:256 returns FULL.
3 reliability-performance-001 — Dedupe GET path parses possibly truncated stdout without checking stdoutTruncated src/tools/pr-review-submission.ts:319-324 DOWNGRADED to MEDIUM by critic Runner overflow safeguards (external-tool-runner.ts:455-467,601-609) kill overflow; only completed/zero-exit truncation escapes. Original HIGH severity overclaimed.

MEDIUM severity — disposition after critic challenge

Finding Reviewer Critic
correctness-state-003 (substring/empty-ID dedupe at R:117) CONFIRMED UPHELD
correctness-state-004 (truncation dup of -001) CONFIRMED DOWNGRADED (same defect, lower severity)
correctness-state-005 (ordering deviation at S:450-510) CONFIRMED DISPROVED — ordering cannot bypass later head validation
correctness-state-006 (same path/line + different evidence) NEEDS_MORE_EVIDENCE kept — GitHub 422 behavior not proven locally
correctness-state-007 (abbreviated SHA at S:44,572) CONFIRMED DOWNGRADED — API failure handled
correctness-state-008 (single-page dedupe at S:276-279) CONFIRMED DOWNGRADED — disclosed limitation
tests-falsifiability-001 (renderer test:108-126 lacks distinguishing evidence/first-winner assertions) CONFIRMED DOWNGRADED — coverage gap, not failure
tests-falsifiability-002 (authorization test:239-253 lacks foreign-session/head cases) CONFIRMED DOWNGRADED
tests-falsifiability-003 (fixtures omit evaluated_at at test:66-70) CONFIRMED DOWNGRADED — S:163-164 tolerates it
tests-falsifiability-004 (test:108-126 lacks first-winner assertion) CONFIRMED DOWNGRADED
tests-falsifiability-005 (no adversarial prefix/empty-ID test) CONFIRMED DOWNGRADED
tests-falsifiability-006 (payload write not asserted) DISPROVED confirmed DISPROVED — payload IS exercised via --input read at transport:144-151
tests-falsifiability-007 (non-JSON POST + nothing-new fall-through) CONFIRMED DOWNGRADED
security-trust-002 (repo/PR not provenance-bound before POST) CONFIRMED DOWNGRADED — no exploitation shown
security-trust-003 (first stderr line returned) CONFIRMED DOWNGRADED — no secret leakage demonstrated
security-trust-004 (truncated event coverage ignored) CONFIRMED DOWNGRADED — disclosed retained-tail limitation
reliability-performance-002 (event uses historical records before filtering) CONFIRMED UPHELD
reliability-performance-003 (substring dup of R:117) CONFIRMED UPHELD
reliability-performance-004 (no body cap) NEEDS_MORE_EVIDENCE kept — external GitHub size limit not established
reliability-performance-005 (no submission lock) CONFIRMED UPHELD
reliability-performance-006 (unbounded readFileSync vs registry guard) CONFIRMED UPHELD
reliability-performance-007 (O(findings×comments) scan) CONFIRMED DOWNGRADED — complexity, not failure
reliability-performance-008 (cross-session not verified) CONFIRMED DOWNGRADED — disclosed residual
reliability-performance-009 (crashed-run inference absent) CONFIRMED DOWNGRADED — disclosed residual
reliability-performance-010 (lexical timestamp compare) NEEDS_MORE_EVIDENCE kept — valid writers use ISO strings
reliability-performance-011 (no abortSignal on gh calls) CONFIRMED DOWNGRADED — runner deadlines retained

LOW severity — disposition

Finding Reviewer Critic
security-trust-001 (directory-scoped gate bypass via override) DISPROVED confirmed DISPROVED — override resolves a separate project root by design; gate state is intentionally project-local

Architectural-coherence clean attestations (6, upheld as non-defects)

# Item Rationale
001 Pure renderer separation (render-review-body.ts:1-8) explicit imports, no fs/network/clock
002 Orchestration/transport remains in pr-review-submission.ts:346-648 traced executePrReviewSubmission through renderer + POST
003 Metadata registration is architect-only (tool-metadata.ts:383-387) verified metadata plus registration assertions
004 Manifest thunk coherent (manifest.ts:231) verified import and handler thunk
005 Barrel export exists (index.ts:228-231) confirmed
006 Registry includes submission payload writer (retention-registry.data.ts:1018-1054) confirmed

Why REQUEST_CHANGES

Two HIGH findings independently verified by source reading prevent approval per protocol rule "Never APPROVE a PR with unresolved CRITICAL findings." Both publish misleading review evidence in ordinary production flows:

  1. pr-review-submission.ts:519 + render-review-body.ts:117-131 — Terminal projection is missing. A post_critic DISPROVED or downgraded finding still publishes with its post_explorer status/severity/evidence. reviewEventFor scans all records, so a superseded HIGH can force REQUEST_CHANGES.

  2. pr-review-submission.ts:230-237 — V2 disclosure objects are filtered to zero strings. A PARTIAL run reports FULL coverage. A run that actually settled partial coverage loses that signal.

Recommended fixes

  • Decode production dimension objects in deriveCoverage instead of string-only filter.
  • Project terminal findings before rendering/event selection (write-pr-review-artifact.ts:213-218,739-750 already does this for the writer).
  • Refuse truncated GET context (or fail-closed on stdoutTruncated).
  • Add falsification cases for the gaps the critic downgraded from CONFIRMED to advisory.

Provenance

  • All 37 base findings independently verified against actual source at HEAD bd798482 in working tree E:/OpenCode/opencode-swarmdevpr.
  • Files read: src/tools/pr-review-submission.ts, src/pr-review/render-review-body.ts, src/tools/write-pr-review-artifact.ts, src/pr-review/completion.ts, src/events/core-events.ts, registration files, retention registry, and tests.
  • Two-stage validation: paid_reviewer initial validation, then mega_reviewer critic challenge.
  • paid_critic not used (over budget); mega_reviewer performed the critic challenge.
  • No files written or changed during the review.

Reviewer → Critic (verified) summary

  • 37 candidates reviewed (8 + 7 + 5 + 11 + 6)
  • Reviewer: 32 CONFIRMED · 2 DISPROVED · 3 NEEDS_MORE_EVIDENCE
  • Critic challenge: 2 HIGH UPHELD, 1 HIGH DOWNGRADED to MEDIUM, 14 MEDIUM DOWNGRADED, 2 DISPROVED (overclaim), 3 NEEDS_MORE_EVIDENCE kept, 6 intent-architecture UPHELD as positive observations
  • Final disposition: REQUEST_CHANGES

@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Swarm PR review — REQUEST_CHANGES (run pr3118-20261006061907)

Bound head bd79848 · 10 lanes (6 base dimensions + 4 consolidated micro lanes covering 8 MATCHED risk families; 3 NOT_TRIGGERED with absence evidence; unclassified-risk always-on) → ~155 raw candidates → 15 normalized → 2 independent reviewer shards → 2-pass critic challenge on the HIGHs. Full artifacts in .zcode/pr-review/pr3118-20261006061907/ (local).

Load-bearing finding (HIGH, critic-confirmed 2-pass): PRR-001 — the tool submits findings.jsonl verbatim, but that file ACCUMULATES one record per finding per boundary (write-pr-review-artifact.ts:679 appends post_explorer → post_reviewer → post_critic). The tool never filters to the settled post_critic records and never reads status/critic_status, so: the posted review lists pre-critic findings as live (renderer dedupe keeps the FIRST occurrence — stale severity/evidence), a critic-disproved HIGH still forces REQUEST_CHANGES from its superseded post_reviewer record, and reviewEventFor sweeps superseded severities. Multi-boundary accumulation is the designed common path (SKILL.md:704-722); the nothing-new dedupe makes a wrong first post sticky. Fix direction: restrict rendering + event derivation to post_critic records and consult status/critic_status.

Other VERIFIED findings:

ID Sev Finding
PRR-002 MEDIUM coverage-disclosure V2 writes unresolvedDimensions as OBJECTS; deriveCoverage's string filter is dead → partial coverage degrades to receipt degradations (mislabeled) or false 'Coverage kind: FULL'; the only test fabricates the string shape. Critic downgraded from HIGH: the #2840 gate machinery independently blocks APPROVE regardless of body text.
PRR-003 MEDIUM Dedupe is unanchored substring containment: finding T-1 is silently dropped once T-10 exists in a prior review (false nothing-new or silent partial drop).
PRR-005 MEDIUM repo/pr_number are caller-supplied and never bound to run artifacts (receipt/records carry no target identity; PR_REVIEW gate stores no prUrl) — findings can be posted to any repo/PR carrying the same commit under the host gh credential. Missing binding, not broken control.
PRR-006 MEDIUM Authorization ladder default-allow: cross-session submission passes the GATE arm vacuously (run ownership never verified) — the release fragment discloses only the ABORT arm's cross-session gap, not this one; allowWorkingDirectoryOverride admits a fabricated self-consistent artifact tree.
PRR-007 MEDIUM Dedupe channel gameable: pre-posted review/comment bodies containing known ids suppress findings (denial of publication; needs predictable ids per the skill's F-001 convention).
PRR-009 MEDIUM Abort arm fail-open on I/O: readCoreEvents swallows read errors to empty → 'no aborts' (core-events.ts:374-375 documents the fail-open); not covered by any disclosed limit.
PRR-012 MEDIUM Confirmed test gaps (7/9): args arms assert no type field; receipt evaluated_at never exercised (dropping it passes green); GET endpoints/per_page unasserted (vacuous ternary fakes); PR_FEEDBACK gate mode undriven; corrupt-JSONL + empty-receipt-head arms undriven; abort narrowing predicates undriven.
PRR-013 MEDIUM Drift cluster: unbounded body list → one bad inline comment 422s the WHOLE review; stdoutTruncated never checked → 2MiB-clipped GET silently empties dedupe → duplicate posts; POST timeout not marked blocked; INFO/NONE groups contradict the claimed CRITICAL→LOW; corrupt coverage disclosure degrades to FULL; abort match ignores mode (PR_FEEDBACK abort over-blocks PR_REVIEW).
PRR-004 LOW finding_id/file_line interpolated raw (only evidence single-lined) — @mentions reach published reviews; range file_line (file:10-20) silently excluded from inline comments.
PRR-008 LOW Registry row cells inaccurate for the new writer (crashBehavior 'atomic temp+rename'; no payload bound; readerCitation missing; 'per POST attempt' over-claim).
PRR-011 LOW base_verification 'bound_fallback' MUST-disclose (producer contract) never read/surfaced.
PRR-014 LOW Controller-tool enumeration + harness matrix in swarm-pr-review SKILL.md not updated with pr_review_submission.
PRR-015 LOW Unguarded throws (corrupt gate state — persistence.ts throws; validateSwarmPath on 'run..' — probe-confirmed; payload write) escape to the generic execution_error envelope; sibling wraps the same call.

REJECTED (transparency): PRR-010 mid-window TOCTOU (precondition pairing closes every realistic interleaving); empty-id dedupe catastrophe (unreachable through the schema-validated writer); Windows-backslash parseLocation failure; ISO-precision mis-sort (all writers emit uniform millisecond-Z); PARTIAL-empty→FULL (unreachable); registry 'rot-detector blind' + coverage-disclosure-unregistered sub-claims (capability does not exist / pre-existing).

Obligation check: the five ACs of #3096 are met as written; PRR-001 undermines AC4's dedupe/consolidation intent for multi-boundary files. Micro-lane attestation: 11/11 families (8 MATCHED attested, 3 NOT_TRIGGERED with absence evidence). Fix handoff is prepared; per the operator mandate, fixes follow via swarm-pr-feedback.

…ssion ladder (PRR-001..015)

Resolves all validated findings from the swarm-pr-review run
pr3118-20261006061907 (REQUEST_CHANGES):
- PRR-001 HIGH: render/derive the event from settled post_critic records
  only (findings.jsonl accumulates per-boundary records); DISPROVED records
  are never published; NONE-severity settled findings are counted and omitted.
- PRR-002: coverage-disclosure V2 object shape and V1 missingDimension are
  honored; a corrupt disclosure refuses instead of degrading to FULL.
- PRR-003/007: dedupe keys on the rendered '[<id>] ' marker (anchored) —
  substring collisions and gameable bare-id pre-posting never suppress.
- PRR-004: finding_id/file_line single-lined and length-capped.
- PRR-006/005: gate-arm cross-session residue + unbound target identity
  disclosed in the release fragment.
- PRR-009: abort scan fails closed (aborted-indeterminate) on an unreadable
  or truncated events store; PR_FEEDBACK aborts no longer over-block.
- PRR-011: base_verification bound_fallback surfaced in the body.
- PRR-013: stdoutTruncated dedupe GETs refuse; POST failure arms marked
  blocked; body capped at 60000 chars with disclosure.
- PRR-015: gate-state read and payload write failures return typed
  refusals instead of the generic execution_error envelope.
- PRR-008/012: registry row cells corrected for the actual writer;
  feedback tests added (multi-boundary, V2/V1 disclosure, corrupt
  disclosure, abort indeterminate, PR_FEEDBACK gate, GET endpoints,
  type assertions).
Frozen check C4 amended (CHECK_WRONG) to the anchored-marker dedupe
semantics; superseding anchor published on issue #3096.
… status markers, reviewer-required test arms

Scope (resolves exactly these review items, nothing more):
- PRR-015: safePath() wraps all validateSwarmPath call sites (trigger-eval,
  findings, coverage-disclosure, payload) into typed 'invalid-args' refusals;
  the dead unwrapped path helpers are deleted and the PRR-015 probe
  (run_id 'a..' -> invalid-args) is pinned by a test.
- PRR-001 (second half): renderer emits an explicit singleLine-flattened
  status marker for non-CONFIRMED findings, e.g. '(pre_existing)'.
- PRR-008 (residue): retention registry gains readerCitation + readBound
  payload sentence for submission-payload.json, citing the write site.
- PRR-014: pr_review_submission added to the SKILL.md:74 controller list
  (2024-line ratchet preserved; harness matrix rows carry no tool column);
  swarm-contract-digest stamp refreshed via stamp-skill-contracts.ts.
- PRR-012/FB-012: schema-binding follow-up filed and corrected on issue
  #3097 (gh comment 6025732799) — payload shape {commit_id, body, event,
  comments}; dismissed_findings/body_truncated are call results, not
  persisted. No code change in this commit.
- PRR-013 gaps named by the reviewer: 10 new tests in
  pr-review-submission-feedback-round2.test.ts — the 7 reviewer-named
  FB-010 arms (evaluated_at receipt term incl. negative control, corrupt
  findings.jsonl, absent pr_head_sha, abort narrowing on foreign
  session/head, truncated dedupe GET, body size cap, V1 disclosure shape),
  plus the PRR-015 probe, a mixed valid+corrupt findings.jsonl arm, and
  the transport coverage fixture re-seeded to the real V2 object shape.

Not addressed in this commit (documented in the closure ledger): PRR-004 and
PRR-011 LOWs (already-disclosed classes); PRR-010 rejected by the critic.

Verification on this committed tree: 49 pass / 0 fail across the 4 touched
suites (pr-review-submission, -transport, -feedback-round2,
render-review-body); typecheck clean; biome ci . exit 0; check:retention,
check:registry-citations, check:invariants, check-tool-registration,
check:test-file-cap, check:test-clock, drift:check --enforce all pass;
tests/unit/skills 628 pass / 0 fail.
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Feedback round 2 — resolution record (head 1f5a4e0)

The round-1 re-review returned NEEDS_REVISION with 3 Important findings plus recommendations. All are resolved at head 1f5a4e0a8:

  1. evaluated_at arm was non-discriminating — rewritten: receipt evaluated_at (02:00) now postdates recorded_at (01:00) with an abort between them (01:30) expecting success, plus a negative arm aborting at 02:30 expecting refusal. Deleting the evaluated_at settlement term turns the positive arm red (verified by inspection in review round 2).
  2. [Workstream C] PR 2 of 2: Post-abort partial-results export bound to receipt-authenticated findings #3097 payload schema misstated — comment 6025732799 amended: the persisted artifact is { commit_id, body, event, comments }; dismissed_findings/body_truncated are call results, not persisted fields.
  3. Status marker bypassed singleLine — the marker now renders singleLine(status, 64).toLowerCase(); the renderer's negative control strengthened to not.toContain('(confirmed)').

Also applied (reviewer recommendations + nits): PRR-015 probe pinned (run_id: 'a..' → typed invalid-args, zero transport calls), mixed valid+corrupt findings.jsonl arm (refuses not-settled, no partial publish), dead unwrapped path helpers deleted (no validateSwarmPath call remains outside safePath), safePath blocked flag aligned with step-0 refusals, registry readBound citation re-targeted to the payload write site, round-2 suite added to the release fragment, and the swarm-contract-digest stamp refreshed after the SKILL.md:74 edit (drift-check --enforce exit 0).

Gates on the committed head: 49 pass / 0 fail across the 4 touched suites (submission, transport, feedback-round2, render-review-body), tests/unit/skills 628 pass, typecheck clean, biome ci . exit 0, check:retention / registry-citations / invariants / tool-registration / test-file-cap / test-clock all pass.

Gating chain for this round: independent reviewer round 2 (NEEDS_REVISION → fixes) → separate final critic (challenged the closure claim; its two completion conditions — stamp refresh + accurate commit counts — are both applied; critic found no code-level defects).

Not addressed, per the round-1 closure ledger: PRR-004 and PRR-011 LOWs (already-disclosed classes) and PRR-010 (rejected by the critic).

@zaxbysauce
zaxbysauce enabled auto-merge October 6, 2026 21:59
…ss gate

CI round 1 on 1f5a4e0 failed unit (macos-latest, 1|2): the G2 evidence
gate (swarm-write-cache-evidence-class.test.ts) cannot fold a write target
that is a property read off a union object (payloadPath.path), so the
submission-payload.json write site was reported as an unfoldable
.swarm-path write. Replace safePath at that one site with a single-return
helper (payloadArtifactPath) + try/catch into the identical typed
'invalid-args' refusal — the shape the engine resolves — keeping safePath
for the three read sites.

Verification: G2 gate 25 pass / 0 fail (--timeout 120000), touched suites
49 pass / 0 fail, typecheck clean, biome ci on the file exit 0.
@zaxbysauce
zaxbysauce added this pull request to the merge queue Oct 7, 2026
Merged via the queue into main with commit 7dc508c Oct 7, 2026
84 of 86 checks passed
@zaxbysauce

Copy link
Copy Markdown
Collaborator Author

Closure ledger — swarm-pr-feedback complete (merged as 7dc508c)

Full disposition of every finding from the Profile B review (REQUEST_CHANGES, comment 6024779853). Nothing silently dropped.

Finding Disposition Evidence
PRR-001 (HIGH) submit-all-records + no status marker RESOLVED settled post_critic-only filter in 0d12d11; non-CONFIRMED status markers (singleLine(status,64)) in 1f5a4e0; multi-boundary test + marker tests
PRR-002 corrupt disclosure degrades to FULL RESOLVED deriveCoverage refuses on corrupt disclosure (0d12d11); V2-object + V1-shape mapping tests (1f5a4e0)
PRR-003 unanchored marker dedupe RESOLVED anchored [id]-prefixed marker matching + bare-id/substring non-suppression tests (0d12d11); frozen C4 amended (CHECK_WRONG) with fresh anchor on issue #3096
PRR-004 (LOW, disclosed class) NOT ADDRESSED — already disclosed in the fragment; documented deliberately
PRR-005 target identity unbound RESOLVED disclosed limit in fragment + schema-binding follow-up on issue #3097 (comment 6025732799, amended)
PRR-006 RESOLVED renderer/transport hardening (0d12d11)
PRR-007 marker semantics RESOLVED same anchored-dedupe change as PRR-003
PRR-008 retention registry narrative RESOLVED writerCitation reworded + readerCitation + readBound citing the payload write site (0d12d11, 1f5a4e0); check:registry-citations green
PRR-009 abort scan fail-open / mode-blind RESOLVED AbortScanResult fail-closed on unreadable stores + mode-match narrowing (0d12d11)
PRR-010 REJECTED by critic (evidence-backed)
PRR-011 (LOW, disclosed class) NOT ADDRESSED — documented deliberately
PRR-012 schema binding for #3097 RESOLVED comment 6025732799: renderer authority, payload {commit_id, body, event, comments}, receipt binding, abort narrowing
PRR-013 test gaps RESOLVED 10 tests in pr-review-submission-feedback-round2.test.ts (7 reviewer-named FB-010 arms incl. discriminating evaluated_at anchor + negative, PRR-015 probe, mixed-corrupt) + transport fixture re-seeded to V2 objects
PRR-014 skill surface RESOLVED SKILL.md:74 controller list (2024-line ratchet preserved) + swarm-contract-digest stamp refreshed
PRR-015 path escapes as execution_error RESOLVED safePath typed invalid-args refusals at read sites; single-return-helper + try/catch at the payload write site (G2-foldable shape); run_id: 'a..' probe pinned by test

Gating chain: 10-lane Profile B review → synthesis → feedback round 1 (0d12d11, 9/15) → reviewer round 2 (NEEDS_REVISION, 3 findings) → fixes (1f5a4e0) → separate final critic (2 completion conditions, both met, no code defects) → CI round 1 caught the G2 evidence-gate fold break (fixed 2fe51d9) → CI round 2 green after one diff-foreign Windows flake rerun → merge queue first round: CI + PR Standards + drift-check all success → merged 7dc508c at 2026-10-07T01:09:51Z. Issue #3096 auto-closed COMPLETED.

Process notes: the round-1 commit message overclaim was corrected in round 2 (exact scope + explicit not-addressed list); both CI failures were diagnosed via job logs before any requeue; the windows-5 rerun was justified by zero import reachability plus a local green on the same head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Workstream C] PR 1 of 2: Controller-mediated, head-bound PR review submission tool

2 participants