build: re-fork Pinvou on v0.9.12 - #44
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if drift: | ||
| print(f"limit drift detected ({len(drift)}):") | ||
| for path in drift: | ||
| print(f" - {path}") |
| if drift: | ||
| print(f"limit drift detected ({len(drift)}):") | ||
| for path in drift: | ||
| print(f" - {path}") |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Multi-agent review: foundation-side findingsReviewed Overall: this is a genuine rewrite on v0.9.12 semantics, not a mechanical transplant — steering is rebuilt on the upstream mailbox with epoch isolation, prompt ownership is woven into the new StaticPromptCtx/fragment architecture, Automation gains session-scoped conversation keys. Deviations from r13 are consistently stricter (Full Access re-blocks non-bypassable registered tools, ambient prompt channels closed, ambient fleet roster removed). No exploitable approval bypass, permission escape, secret leak, or cancellation/cleanup bug was found: exact-dispatch is enforced at every dispatch seam with forged-backend fail-closed tests, redaction is a whitelist projection, the steering register re-checks after the permit await (TOCTOU closed), and 🔴 Blocker — #32 eval-control semantics are gone without a clean disposal (cross-repo decision needed)
Please either re-implement the control semantics here (with tests + fingerprints restored), or record a deliberate drop in the register and fix the parent's dangling calls accordingly. 🟠 Major — safety claims with zero test coverage
Suggest restoring behavior tests for at least the write limit and bulk cancel before this becomes the protected baseline. 🟡 Minor
✅ Verified preserved (sampled with evidence)Narrow facade + route limits + wire/embedding aliases; reliable steering (opaque ids, committed/dropped, withdraw outcome, |
Independent review addendum: findings not in the earlier listI independently reviewed MajorUpstream tests flipped/replaced without the fork-policy §3.4 annotation. The policy requires stating why an upstream test no longer holds when product semantics change it. At least three cases are silent:
Minors
|
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Review: request changes
Consolidated multi-agent deep review of v0.9.12..b4c02616. Ground truth verified independently: anchor = dcd4c200 (= v0.9.12), 5 linear commits above it, 62 files +3127/−614, only crates/ touched, no merge commits, no lockfile/CI/submodule changes, no secrets in the delta. The rewrite is genuine (not a transplant), the four retained topics are real, and no smuggled scope was found in any of the 62 files. I am requesting changes for the disposal gaps, the coverage collapse, and the transition issues below. This confirms/extends the two earlier comments where noted and adds new findings; the first two items now carry compile-level evidence.
🔴 Blockers
B1 — #32 eval-control disposal is incomplete, and the register is false about it.
benchmark-observability/benchmark-eval-controlsremain empty feature shells with zerocfgconsumers; all sixforkguard_benchmark_*tests are gone.- The parent's
benchmark-hooksbuild does not compile against this head:bridge.rs:2363-2366callswith_final_only_after_tool_budget()/with_missing_read_action_repair(), andTurnToolSecurityPolicyhere has onlynew/with_read_only_dispatch/with_trusted_hooks(core/ops.rs:73-102). Verified by an actualcargo check --features benchmark-hooksof the Hmbown#453 tree with the submodule pinned tob4c02616b: 2× E0599. PR Hmbown#453 does not touch those calls. - The register row “评测隔离留在父仓
benchmark-hooks” is factually wrong — nothing named eval isolation survives on either side (and Hmbown#453 simultaneously deletes the only CI lanes that compiled that feature). - Either re-implement the control semantics here (tests + fingerprints), or record a deliberate drop in the register and remove the dangling parent calls in Hmbown#453 in the same pairing.
B2 — #35 keyless chain tail: the loss is worse than registered.
- The v0.9.12 tail is DuckDuckGo (
web/backend.rs:119-122); Bing exists only as an internal fallback that fires after a successful HTTP response (bot challenge / zero parseable results,web_search.rs:1424-1490). A mainland network-level DDG failure (DNS poisoning, SNI reset) errors at.send()and never reaches Bing. - Net effect: Metaso/Bocha/Baidu API-backend users lose keyless fallback entirely on mainland networks.
forkguard_api_provider_chain_tail_is_bingwas deleted with no replacement; the register buries this in the generic “旧产品搜索覆盖” bucket; the parent'sprefs/search.rs:16-21comments are now doubly stale (the foundation default is also now Firecrawl). - Restore a Bing tail (or fall back to Bing on network-level errors), or register the semantics change honestly and fix the parent comments in Hmbown#453.
🟠 Must-fix before this becomes the protected baseline
M1 — Coverage collapse 63 → 18. The 18 kept tests are genuinely result-oriented, but the following surfaces are now implementation-untested (each had an r13 guard): 64 KiB write cap (both call sites), bulk cancel idempotency/session scope, terminal cleanup (delete_terminal_task refusal + delete_terminal_run identity re-check — r13 even asserted get_task fails afterwards), ExtraTools all-mode registration, restricted hooks opt-in (allow_hooks, zero refs), session trusted-roots override (zero refs), worker-boundary conversation-key transfer. fork-guard.sh --fast is grep-only and would not catch a refactor that bypasses the call sites. Restore at least: write cap, bulk cancel, terminal cleanup, ExtraTools registration, plus one end-to-end composer-sealing test (see M6).
M2 — Upstream test flips still lack the fork-policy §3.4 annotation. Confirmed for full_access_* (engine/tests.rs:12212) and the skills merge-path replacement (skills/tests.rs:1105). One factual correction to my earlier comment: system_prompt_merges_workspace_and_configured_skills_dir was renamed + inverted in place as system_prompt_uses_only_explicit_configured_skills_dir (prompts.rs:2019), not deleted — the annotation gap stands, the wording does not. Two benign renames into forkguard supersets (automation enqueue :2823, task schema :3556) deserve a line each for traceability.
M3 — [NEW] settle_steers_on_interrupt can silently drop accepted steer input (engine.rs:3778-3792). Whenever take_stopped returns any id, the channel is drained indiscriminately (while try_recv().is_ok() {}). Safe for StopDropInbox (all in-flight ids are in dropped), but under a prior unsettled StopDropInbox + an InterruptKeepInbox interrupt, current-generation registered steers sitting in the channel are discarded while their entries stay unsettled — the input is lost with no SteerCommitted/SteerDropped event, violating the PR's own no-silent-loss contract, and the unsettled entry leaks until session retire. Precedents are narrow but real (a cancel landing in the terminal tail after the loop's last iteration). Fix is mechanical: discard only ids present in dropped; re-queue the rest into pending_steers.
M4 — this PR alone does not close pinvou-agent#254. The shared bare CancellationToken slot remains (engine.rs:936, reset_cancel_token, cancel_with_mode with zero identity check); of the three autonomous self-start paths only goal continuation got a boundary re-check (engine.rs:2937-2944); idle-completion and shell-wake remain exposed. CodeWhale#38 (TurnCancelSlot + cancel_turn + publish_stop_disposition) therefore needs full re-authoring on this base — its merge-base with this branch is v0.9.5, with 7 touchpoints in engine.rs plus a handle.rs rewrite. Please state that explicitly in the register/transition plan so downstream (#38/Hmbown#408) doesn't plan a rebase.
M5 — facade: widened where it should be enumerated. 18 mod → pub mod (the earlier "36" counted diff lines). Every flipped module is consumed by the parent today, but pub mod core / pub mod mcp expose entire recursive APIs versus the ~100 symbols the host actually uses. The tui privatization + root AppMode/ApprovalMode re-exports is the right pattern — apply it to the other 17, or switch to an explicit re-export list. Note the privatization breaks the current parent tree (21 uses / 16 files of deepseek_tui::tui::…); Hmbown#453 migrates them, but the register only says “narrow AppMode/ApprovalMode API published to hosts” — name the import migration requirement explicitly.
M6 — ambient sealing has no drift guard. The six if static_composer_installed() guard clauses are scattered through the ~300-line compose function, so any upstream addition of a new ambient block defaults to unsealed with no compile-time signal, and no test asserts the sealed composition end-to-end (the renamed skills test sets the flag without a composer installed). One forkguard test — composer installed ⇒ prompt contains none of AGENTS.md / constitution.json / .cursorrules / pack / harness content — would pin the doctrine. Related: .codewhale/handoff.md relay (prompts.rs:1276-1278, fn :285-305) stays open under composer ownership; seal it or document the exemption.
🟡 Minor (new, non-blocking)
- “Five signed Pinvou commits”: no cryptographic signatures exist (
%G?= N ×5); only DCO trailers. Reword or sign. - Commit 5 measured: 50 files, +2491 ≈ 80% of all additions, pure lint ≈ 6–8%; all five bodies are subject + trailer only. The body's “release-lint compatibility folded into the final topic commit” materially understates it and per-commit bisect value is ~0. The protected-ref transition is the last cheap window to re-cut per-topic commits with bodies.
- 64 KiB cap scope:
File action=patch create_if_missingwrites unbounded new files (apply_patch.rs:405, 621-642; parity with r13,edituncapped by design) — state the cap's actual scope (write-tool content only) in the register. authority.rs:449-462: the resolver'sAllowbranch for non-bypassable + auto-approve is now dead in production (the fork blocks earlier inturn_loop), but the doc still teaches the old behavior — a future caller would silently reintroduce auto-approve. Update or delete.- Fleet profiles: prompt profiles installed via
Op::SetFleetRosterare silently ignored at spawn (subagent/mod.rs:9108, 12880re-derive from session config — two sources of truth), and the profile prompt overlay is uncapped (:12950-12970) versus the 100 KiB discipline elsewhere. - Manual/agent-triggered automation runs bypass the no-overlap guard (
run_now_with,automation_manager.rs:1719-1756; r13-inherited) — one register sentence. EmbeddingHostreusesMessageId::StatusContextSourceConfigured(status.rs:378) — the status UI cannot distinguish a host-imposed limit from a user-configured one.- The r9/r13 turn-scoped foreground-shell kill on cancel has no foundation-level successor (only session-scoped
kill_for_session); product behavior now rests solely on the parent'sSessionTurnShellTasks. Register the disposal. - The rewritten fork-guard anchors no native-search fingerprint (old
documented_server_side_web_search_for_route/is_exact_url_routeanchors dropped with the upstreaming) — one line in the register. - r13 topic #28 cannot be classified from any ledger — needs an author note (kept/renamed/dropped).
✅ Verified good (sampled with evidence)
- Scope hygiene: every one of the 62 files classifies into T1–T4 / mechanical v0.9.12 API adaptation / lint (including the 15 suspicious "new-only" files, checked hunk-by-hunk). Nothing to split out.
- T2 security: no exploitable bypass found. Exact-dispatch enforced at a single chokepoint before lock acquisition; subagent, MCP-direct, interpreter, retry/fallback, ACP/HTTP seams all unreachable or gated while latched; policy non-serializable (wire cannot mint authority); wire projection is a compile-time-exhaustive whitelist under the wildcard-ban lint; redaction is whitelist projection with no leak path found; MCP secret resolver covers all five env sites incl. OAuth headers.
- T4: conversation keys, no-backfill/no-overlap scheduling, and the v2/v3/v4 + v5-fail-closed matrix verified race-free including the rollback direction (new-written v3 readable by both r13 and upstream).
- Native search (#33) upstreaming verified at symbol level, including the K3 180 s / 8-call budgets and exact four-way endpoint matching.
- Steering TOCTOU re-check ordering is real (
reserve_steer: register re-validatesactive_targetafter the permit await); child settlement is deterministic (TurnMailboxBarrier, pre-cancel park flag); shell process-group kill paths byte-equivalent to upstream.
Related PRs (scheduling notes, not part of this verdict)
- Close as obsolete: #40 (fully upstreamed incl. its own test, renamed
tool_context_for_call_preserves_turn_and_sets_call_origin), #34 (v0.9.12's encoding_rs decode loop emits the valid prefix + U+FFFD; legacy-decoder leg moot), #39 (applying it to this base would make the model-facing docs false — re-file only if B2 is resolved by restoring the Bing tail). - Rebase: #20 → #42 → #43 (low conflict; #43 is under active review).
- Rewrite: #31 (notify fix already upstream; keep finance gating + verify/verifier wording), #37 (matcher ports, engine wiring collides semantically with upstream's
exec_shell_ask_rule_decision_for_policy), #41 (fresh accuracy pass last). - Re-author #38 after #43/#37 settle (see M4).
- Parent Hmbown#453 must fix before pairing: benchmark-hooks compile (B1 +
forwarder.rs:1240-1260TurnUsage→first_token_ms/request_ms),pinvou-cli/Cargo.lockregeneration, restore a benchmark-hooks compile lane (the upgrade PR deletes the only ones), search.rs comments, register additions (tui privatization migration,execute_with_options_env_for_owner→_and_sessionvisibility, MCP hot-deny disposal, manual-trigger overlap caveat).
Happy to re-review on the follow-up push; the disposal-ledger items (B1/B2, the register corrections, and #28) are the critical path to the protected-baseline transition.
asto18089
left a comment
asto18089
left a comment
There was a problem hiding this comment.
Comment from the parent-side review of Pinvou/pinvou-agent#453
Context: Hmbown#453 pins the CodeWhale gitlink at this PR's head b4c02616 and was reviewed in depth on the parent side. The fork work itself is high quality — 5 well-scoped commits, 18 forkguard_* behavior tests mapped 1:1 to features, and the security core is strengthened (non-serializable ExactToolDispatchPolicy prevents transcript-forged authority, single final-dispatch enforcement point, forged-backend-name tests).
Three things must be resolved in this PR before the parent can proceed:
- CodeQL check is failing with 1 high alert introduced by this PR ("2 configurations not found" warning + "New alerts: 1 high"). The parent PR's verification section does not currently disclose this. Please resolve or disprove the alert before merge.
- No human review yet — only advanced-security bot comments. Since this replaces the entire fork baseline, at least one approving human review should land here first.
- Publish
pinvou-v0.9.12-r1immediately after merge and complete thepinvou3-cleanprotected-branch transition (which needs an explicit maintainer decision — normal merge is impossible since the histories are disjoint). Until the tag exists, the parent's gitlink pins a SHA reachable only from a force-pushable branch, and the parent'sverify-public-submodulegate is designed to require branch = tag = gitlink.
Non-blocking notes for the fork register (update here or in a fast-follow):
- Plan mode also receives host extra-tools injection (
append_host_extra_toolsis called in the Plan branch too) — acceptable under the host contract, but record it explicitly in fork-modifications. WriteFileToolgained a fork-specific 64 KiB single-write cap (WRITE_FILE_MAX_CONTENT_BYTES) — a behavior difference vs upstream worth documenting.benchmark-eval-controlsincrates/tui/Cargo.tomlis now an empty feature shell (0 gated code sites, 0 tests) behind a stale comment — remove it with the benchmark topic cleanup.- Automation due-sweep changed
list_runs(&id, Some(25))→list_runs(&id, None)— O(all runs) file IO per sweep; fine under retention pruning, but worth keeping an eye on. - Note for downstream: the issue-Hmbown#254 turn-bound cancel chain (CW#38, candidate
ef424c1d7on the r13 line) has no disposition in the upgrade report or register; the r1 engine still swaps an identity-less shared cancel token (engine.rs:1543). Either argue closure or plan the re-port — this needs an explicit decision on the parent side.
|
已按原评审及补充评审逐项复核，结论如下。 确认存在并已修复
以上最终落在 确认是债务，但本轮不做破坏性热修
不属于本 fork 修复范围
验证
仍有一个发布流程门禁：公开 |
|
补充对当前 CodeQL 汇总红灯的完整核验：该 check 报告 101 条 annotations（汇总为 151 alerts）。我按 annotation path 和精确行号逐一与官方
因此现有 CodeQL 汇总失败是 PR base 仍为旧 r13、clean re-fork 造成超大比较面后暴露的上游告警；当前 annotations 中没有一条能归因到本 fork 新增行。后续仍应由上游安全治理处置这些告警，但不应在 Pinvou clean re-fork 中复制 101 条私有补丁。公开 baseline 更新后，也应让 CodeQL 以正确基线重新计算增量。 |
Re-review of
|
|
已核验本轮复审结论，并在 确认存在并已修复
确认是有意设计，已登记但不改实现
验证结果：CodeWhale 默认完整测试 11,703 passed / 0 failed / 12 ignored；forkguard 默认 30/30、 |
Re-review of
|
f853f8f to
ff299f9
Compare
|
Post-merge review follow-up (from the Pinvou/pinvou-agent#453 audit): this branch silently dropped overdue FREQ=ONCE automations, lost the actionable search-failure hint from the old fork line, and had a fail-unsafe deletion order in delete_terminal_task. Fixes are in #47. Also note this PR's description is stale: it claims 5 commits / 62 files / +3127/-614, but the merged head was 8 commits / 72 files / +4848/-637 — worth correcting for the record. |
…est clippy (#60) * chore(baseline): resync the tui changelog slice, surface facts, and test clippy Split out of CodeWhale#56 per CONTRIBUTING.md: changelog entries are written on main at merge time and PRs carrying changelog hunks are asked to strip them, so the DDG-to-Bing degradation entry (behavior already merged in #44 via ff299f9; the root CHANGELOG already carried it, the packaged tui slice did not) moves here together with its generated web twin. The surface-facts trio moves with it because no gate on the pinvou3-clean lane checks them; note v0.9.13 published on 2026-09-14, so the next receipts run should refresh latestPublishedRelease again. The redundant-closure cleanup in engine/tests.rs rides here because it blocks the master/main and release lint lanes, not the fork lane. Signed-off-by: asto18089 <asto18089@126.com> * fix(web): evolve telemetry trust prose with the release bump The surface-facts contract interpolated latestPublishedRelease.version into a required 'published <version> release asked first' phrase. Once the pin moves to v0.9.12 that demanded phrase becomes false: 0.9.12 counts by default and never asked. Name the historically fixed asking release instead, matching the contract's upstream evolution: prose says 'earlier 0.9.11 release asked first' (en + zh), the test asserts it as a literal, and the roadmap/faq/telemetryLead copies follow. Catalogs regenerated with npm run i18n:gt -- export; npm test 407/407 green. Signed-off-by: asto <asto18089@126.com> * test(web): anchor the asking-release phrase across site faces Review follow-up: the telemetry trust prose this PR evolves is mirrored across the en/zh dictionaries and the faq/roadmap pages with no test anchor, so a future edit could update one face and silently leave the others behind. Assert the fixed 'earlier 0.9.11 release asked first' phrasing (and its zh mirror) in every face and reject the interpolated 'published 0.9.11' wording; make providerCountDefinition version-free ('the providers of any published release') so the same stale-pin drift cannot recur there; refresh the stale comment above the telemetry assertions. Signed-off-by: asto18089 <asto18089@126.com> --------- Signed-off-by: asto18089 <asto18089@126.com> Signed-off-by: asto <asto18089@126.com>
3 participants
Summary
Re-fork Pinvou's maintained CodeWhale foundation directly from upstream
v0.9.12and preserve only the four host lifecycle topics that remain necessary.The merged head reduces the old v0.9.5 r13 fork from 110 changed files and
+10895/-1195to 72 files and+4848/-637, while retaining result-oriented regression coverage.Changes
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --lockedwith the documented CI allow list — warning-freeRUST_MIN_STACK=16777216 cargo test --workspace --all-features --locked -- --test-threads=1— all crate, integration, PTY/Cucumber, and doc tests passedcargo check --workspace --all-targets --lockedcargo test -p codewhale-protocol --test parity_protocol --locked— 17 passedcargo test -p codewhale-state --test parity_state --locked— 6 passed./scripts/release/check-ohos-deps.shforkguard_*behavior tests passed: 30 default plus 6 feature-gated benchmark controlsv0.9.12..HEADfound no leaksImpact and notes
v0.9.12commitdcd4c200f72f0c1ffd60d8e7f6850313db879fc5and contains eight DCO-signed Pinvou commits above it.ff299f94b0795180c76d0336152385dbd02dfa05. Exact-head release-gate fixes and final protected-ref/tag publication are tracked in follow-up PR fix(ci): close exact-head release gates #46.Checklist