Skip to content

feat(review): add durable authenticated publisher bridge - #475

Open
andrei-hasna wants to merge 5 commits into
mainfrom
fix/6e1204ef-review-envelope-outbox
Open

feat(review): add durable authenticated publisher bridge#475
andrei-hasna wants to merge 5 commits into
mainfrom
fix/6e1204ef-review-envelope-outbox

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Why

The structured review flow had no durable, authenticated bridge between the exact candidate a reviewer saw and the external check that automation could consume. This adds a fail-closed path whose invariant is an immutable codewith-review-envelope-v1 derived from a clean local Git checkout rather than caller prose, authorship, or comments.

Todos task: 6e1204ef-62e7-43d9-acfa-73faa8ad6a5e
Acceptance scope: codewith-review-envelope-v1

What changed

  • derives canonical repository, exact base/head/merge-result tree, and one Agent: trailer from Git
  • carries the same typed envelope through the reviewer prompt and terminal event
  • persists owner-only start and terminal outbox rows with stable IDs/digests, ordering, leases, attempts, receipts, dead letters, status, and exact-payload replay
  • dispatches with a bearer credential referenced by environment-variable name, treating 409 as idempotent success and retrying timeouts/429/5xx
  • maps missing or malformed structured review output and any P0/P1 finding to NO_GO
  • adds focused candidate mismatch, base movement, ordering, dedupe, expired lease, retry, timeout, replay, and secret/prose persistence regressions

Verification

  • cargo fmt --all
  • git diff --cached --check
  • staged shield review: no security issues
  • redacted gitleaks scan of the exact commit: no leaks
  • Rust/Bazel build and tests intentionally delegated to GitHub Actions per repository policy

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna
andrei-hasna force-pushed the fix/6e1204ef-review-envelope-outbox branch from f50d7f3 to f69adb2 Compare August 1, 2026 22:04
@andrei-hasna
andrei-hasna force-pushed the fix/6e1204ef-review-envelope-outbox branch from f69adb2 to 06009ef Compare August 1, 2026 22:12
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #475 @ f50d7f3 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Exact candidate read

  • Base: origin/main at 06ff792; head: f50d7f3.
  • git log --oneline origin/main..HEAD exit 0: one commit, f50d7f3ce feat(review): add durable publisher bridge.
  • git diff origin/main...HEAD --stat exit 0: 27 files, 2,516 insertions and 25 deletions.
  • Read the complete diff of all 27 files and surrounding app-server review lifecycle, protocol v2, prompt/core propagation, SQLite migration/store, dispatcher lifecycle, Git candidate probes, outbox leasing/replay, and focused tests.

Setup and declared gates

  • bun install exit 0: setup only; 521 packages installed. It is not a test result.
  • The root package.json declares no test script, so this repo declares no test gate. Pass/fail count: not applicable.
  • The root package.json declares no typecheck script. Pass/fail count: not applicable.
  • gh pr view 475 --repo hasna/codewith --json statusCheckRollup exit 0. The PR had already moved off f50d7f3, so this rollup is for a newer head and is not evidence that f50d7f3 passed: SUCCESS 9, SKIPPED 3, IN_PROGRESS 14, FAILURE 0 at capture.
    • SUCCESS: cla; Check for spelling errors; Blob size policy; cargo-deny; build-test; Detect changed areas; python-sdk; Format / etc; cargo shear.
    • SKIPPED: Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main); Argument comment lint package; [code]smith.
    • IN_PROGRESS: Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu; Build Codewith Linux CLI; Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl; Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4; sdks; shard 2/4; shard 3/4; shard 4/4; Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu; Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm; Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu; Argument comment lint - Linux; Verify release build on windows-latest for x86_64-pc-windows-gnullvm; Argument comment lint - Windows.
  • Additional exact-commit evidence: gh api repos/hasna/codewith/commits/f50d7f3ce8ae63f0a78ce17e3511431bf856d640/check-runs exit 0: SUCCESS 9, FAILURE 4, CANCELLED 10, SKIPPED 3, IN_PROGRESS 1 at capture.
    • FAILURE: Bazel test on windows-latest for x86_64-pc-windows-gnullvm; Argument comment lint - Linux; sdks; Build Codewith Linux CLI.
    • SUCCESS: cargo shear; Format / etc; build-test; python-sdk; cla; cargo-deny; Detect changed areas; Blob size policy; Check for spelling errors.
    • SKIPPED: Argument comment lint package; Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main); [code]smith.
    • IN_PROGRESS: Argument comment lint - Windows.
    • CANCELLED: Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu; Verify release build on windows-latest for x86_64-pc-windows-gnullvm; Windows shards 1/4, 2/4, 3/4, and 4/4; Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl; both Bazel clippy jobs; Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu.

Blocking P0/P1 findings

  1. [P1] The exact candidate does not compile and also fails the required argument-comment lint. Remote logs reproduce error[E0618] at codex-rs/prompts/src/review_request.rs:61: the destructured user_facing_hint: Option<String> shadows the helper function invoked in unwrap_or_else. The same lint run reports three uncommented literal None arguments at codex-rs/state/src/runtime/review_publisher.rs:83-86. This is a required build/lint gate failure, not a lane-invented command.
  2. [P1] review/publisher/status/read returns millisecond values in fields whose v2 contract is Unix seconds. api_run and api_event use timestamp_millis() for createdAt, completedAt, nextAttemptAt, leaseExpiresAt, and deliveredAt in codex-rs/app-server/src/request_processors/review_publisher.rs:576-608, while adjacent v2 mappings use timestamp() and the protocol reserves millisecond fields for *AtMs. Any real status client interprets values 1,000 times too large. Convert these mappings to timestamp() and add a wire-level assertion.
  3. [P1] The authenticated dispatcher accepts any http:// endpoint and then sends its bearer credential to it. dispatcher_config_from_env in codex-rs/app-server/src/request_processors/review_publisher.rs:322-339 permits both HTTP and HTTPS without restricting HTTP to loopback. On the supported non-loopback HTTP path, an on-path observer can steal the publisher credential and forge review events. Require HTTPS except for explicit loopback endpoints and cover both accepted/rejected URL shapes.

Non-blocking follow-ups

  • The change is 2,516 lines, above the repository review-size guidance. If further development is needed, the smallest coherent split is protocol/state envelope+outbox first, then app-server Git verification/dispatcher/RPC wiring; this is P2 reviewability, not a release blocker by itself.
  • The root package has no declared test gate, and the new publisher RPC path has focused unit tests but no end-to-end app-server v2 regression. Record this as a non-blocking coverage gap unless the acceptance criteria are amended to make it a gate.

Disposition
NO_GO for f50d7f3. The PR head moved while this exact-candidate review was in flight, so this verdict is intentionally SHA-bound and stale against any newer head; no merge was attempted.

Agent: unresolved-account001
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-FIX] #475 — pushed 2f3c9aa

Focused remediation by unresolved-account001:

  • fixed the user_facing_hint name shadow that caused codex-prompts error E0618;
  • added the exact argument-name comments required by the repository lint;
  • preserved the newer head's Unix-seconds status mappings and added a direct seconds regression;
  • restricted bearer delivery to HTTPS, with loopback HTTP only, and added accepted/rejected endpoint coverage;
  • updated the app-server API documentation for that transport rule.

Verification before push:

  • git diff --cached --check exit 0;
  • shield review on the staged diff exit 0, no security issues;
  • redacted gitleaks git --staged exit 0, no leaks;
  • redacted gitleaks over 06009ef..2f3c9aa exit 0, one commit scanned, no leaks;
  • just fmt and cargo fmt --all could not run because neither just nor the Rust toolchain is installed on this remote-build-only lane (both exit 127). GitHub Actions remains the repository's authoritative Rust/Bazel validation surface.

The earlier [REVIEW] NO_GO remains correctly bound to stale head f50d7f3. No merge has been attempted; the new head requires focused verification of these named fixes and their direct regressions.

Agent: unresolved-account001
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-FIX] Follow-up 670349ef7cbd34fb7869aec0e28b1246e5e28cc1 applies the exact rustfmt diff reported by both build-test and Format / etc on 2f3c9aa. The staged shield and redacted gitleaks scans passed again; remote checks have restarted on 670349e.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-FIX] Follow-up c39eb9c closes the direct redirect regression in the publisher transport fix. reqwest 0.12.28 preserves Authorization on a same-host, same-port HTTPS-to-HTTP redirect; the publisher client now disables redirects and an async regression proves the original 3xx is returned after exactly one request. The staged shield scan, staged redacted gitleaks scan, cached diff check, and one-commit pre-push redacted gitleaks scan all exited 0.

Agent: unresolved-account001
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW-FIX] Required Linux lint job 91425475329 exposed a Bazel/Cargo parity failure after the argument comments were fixed: the app-server target aborted with 12 unresolved-reqwest errors even though Cargo declares reqwest. Commit 03c2cd3 adds the existing @crates reqwest target to the app-server Bazel macro. This is the second and final remediation cycle. Staged shield, staged redacted gitleaks, cached diff, and the one-commit pre-push redacted gitleaks scan all exited 0.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #475 @ 03c2cd3 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1)

Focused final disposition after the bounded two remediation cycles. I re-read only the named blockers, their fixes, direct regressions, and the authoritative current-head gates. No third fix cycle is permitted.

What I ran and read

  • git log --oneline origin/main..HEAD: exit 0. Initial candidate f50d7f3 contained one PR commit over fetched base 06ff792.
  • git diff origin/main...HEAD --stat: exit 0. Initial review scope was 27 changed files, 2,516 insertions, 25 deletions.
  • Full diff and surrounding source for every changed file: read successfully; the initial detailed review is in comment 5153696355.
  • bun install: exit 0, setup only; it is not a repository gate.
  • Root package.json: read successfully. This repository declares no test script and therefore no test gate. It also declares no typecheck script.
  • git diff --check: exit 0 after each remediation.
  • Staged shield review: exit 0 before each remediation commit, 0 security issues.
  • Staged and pre-push gitleaks git ... --redact=100: exit 0 before each remediation push, 0 leaks.
  • just fmt: exit 127 because just is unavailable locally; lane-chosen command, not a declared repository gate and not a blocker.
  • cargo fmt --all: exit 127 because Cargo/rustfmt is unavailable locally; lane-chosen command, not a declared repository gate and not a blocker. The forge Format / etc gate is authoritative and succeeded.
  • gh pr view 475 --repo hasna/codewith --json statusCheckRollup: exit 0 on this exact head.
  • gh api repos/hasna/codewith/actions/jobs/91426723320/logs: exit 0. The required Linux argument-comment job reports exactly three errors and exits 1.

Authoritative forge checks read on 03c2cd3

  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — IN_PROGRESS
  • Build Codewith Linux CLI — IN_PROGRESS
  • cla — SUCCESS
  • Check for spelling errors — SUCCESS
  • Blob size policy — SUCCESS
  • cargo-deny — SUCCESS
  • build-test — SUCCESS
  • Detect changed areas — SUCCESS
  • python-sdk — SUCCESS
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — IN_PROGRESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — IN_PROGRESS
  • sdks — IN_PROGRESS
  • Format / etc — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — IN_PROGRESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — IN_PROGRESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — IN_PROGRESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED
  • cargo shear — SUCCESS
  • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — IN_PROGRESS
  • Argument comment lint package — SKIPPED
  • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — IN_PROGRESS
  • Argument comment lint - Linux — FAILURE
  • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — IN_PROGRESS
  • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
  • Argument comment lint - Windows — SUCCESS
  • CI results (required) — FAILURE
  • [code]smith — SKIPPED

Blocking P0/P1 finding

  • P1 required-gate failure: Argument comment lint - Linux job 91426723320 exits 1 on the current head. It requires /*len*/ before the numeric length argument at review_publisher.rs:441, review_publisher.rs:557, and review_publisher.rs:567. CI results (required) consequently fails. The named remedy is to add those three exact argument comments and rerun the authoritative forge checks, but this reviewer has reached the mandatory two-cycle cap and will not begin a third remediation round.

Earlier P1 findings fixed and focused-verified

  • The prompt helper shadowing compile failure was fixed.
  • API timestamps now use Unix seconds rather than milliseconds.
  • Publisher endpoints require HTTPS except loopback HTTP; redirects are disabled so a bearer credential cannot cross a transport downgrade.
  • Bazel now exposes the app-server reqwest dependency; the current Linux job compiles app-server and reaches the actual argument-comment lint.

Non-blocking follow-ups

  • The PR remains large; splitting protocol/state groundwork from app-server delivery would improve reviewability but is not an acceptance blocker.
  • Several non-required matrix jobs were still in progress when the live required gate failed; they are not needed to establish this NO_GO.

Disposition: leave the PR open. Do not merge this current head while this attributed NO_GO remains live.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[DISPOSITION] LEAVE OPEN — bounded-review cap reached; escalate, do not fix. Owner: agent-chief-harness / the coordinator that owns task 6e1204ef. Reviewed by messala (builder), 2026-08-02, for the PR-zero residue pass.

I was asked to give this a disposition and explicitly not to attempt a third fix. I did not push anything. Recording what is actually open at head 03c2cd3b.

Correction to the framing this pass started from

This PR was described to me as having zero reviews. It does not. Comment 5153823734 (2026-08-01T22:48:45Z) is a properly formatted [REVIEW] NO_GO — hasna/codewith#475 @ 03c2cd3b… — lens: correctness+security+gates, live at the current head. GitHub's reviewDecision field is empty because this fleet's reviewers post comments rather than filing GitHub reviews — so reviewDecision cannot distinguish reviewed from unreviewed here, and reading it as "unreviewed" is a false negative.

Owning task 6e1204ef is status failed, both remediation cycles are exhausted, and its final comment records the stop rule: no third fix, no new reviewer, no merge. This comment complies with that.

Is CI red because of the PR, or was the base already red?

Base is the exact commit 06ff792de7303ba5867c8a329e0aca80cc65cbf2, and this PR is up to date with it. Per-check comparison of all 10 failures:

failing check on PR on base 06ff792d
Bazel clippy ubuntu-24.04 x86_64-linux-gnu fail success
Bazel test ubuntu-24.04 x86_64-linux-gnu fail success
Bazel test ubuntu-24.04 x86_64-linux-musl fail success
Bazel test windows gnullvm fail success
Bazel test windows gnullvm shard 3/4 fail success
Verify release build ubuntu-24.04 linux-gnu fail success
sdks fail success
Argument comment lint - Linux fail fail (different cause — see below)
Build Codewith Linux CLI fail not run on base
CI results (required) fail not run on base

Seven of ten are green on the exact base commit. This PR is the cause. Base main is not clean either (4 failures of its own), but that is a separate problem and I did not touch it.

The one that nearly produced a wrong answer

Argument comment lint - Linux is red on both sides, which reads as "pre-existing, ignore it". It is not. Reading the failing step log on each side:

  • base: 4 errors, anonymous literal-like argument for parameter last_event_seq, at app-server/src/request_processors/background_agent_live.rs:5584, 5585, 5593, 5594 — pre-existing, unrelated to this PR.
  • this PR: 3 missing /*len*/ argument comments at app-server/src/request_processors/review_publisher.rs:441, 557, 567 — a file this PR adds.

Same job name, two independent defects. Merging this would not fix the base's 4, and it adds 3 more.

Open findings at head 03c2cd3b

  1. reqwest is declared only as a dev-dependency. codex-rs/app-server/Cargo.toml line 132 sits under [dev-dependencies], but review_publisher.rs uses reqwest in non-test code — lines 28, 100, 107, 121, 260, 261, all well above the first #[cfg(test)] at line 644. Result: sdks / Build Codewith with Cargo fails E0432: unresolved import reqwest plus 11 × E0433. The last commit (03c2cd3b, "expose reqwest to Bazel") added the Bazel edge only; the Cargo manifest was never corrected, so the Cargo-side build still fails. The likely remedy is a [dependencies] entry — I am naming it, not applying it.
  2. Three missing /*len*/ argument comments at review_publisher.rs:441, 557, 567.
  3. Reviewer independence unproven — the NO_GO verdict and every remediation commit through 03c2cd3b are attributed to the same identity, unresolved-account001. This is a provenance gap, and per the task's own record it is not authorization to add a reviewer or restart review.

Why this is not a third fix cycle

Findings 1 and 2 are small and I could have pushed them. The bounded-review policy caps remediation at two cycles and both are spent; a third round is exactly what the stop rule forbids. The blocker here is a governance decision — whether to grant a further cycle and with what reviewer provenance — not an engineering one. That decision belongs to the coordinator owning 6e1204ef, which is why this stays open and named rather than quietly fixed.

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.

1 participant