Skip to content

fix(tui): repair residual model-facing copy (2026-10-07 audit batch) - #136

Open
asto18089 wants to merge 53 commits into
pinvou3-cleanfrom
fix/model-facing-copy-residue
Open

asto18089 wants to merge 53 commits into
pinvou3-cleanfrom
fix/model-facing-copy-residue

Conversation

@asto18089

@asto18089 asto18089 commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Delivers the remaining engine-side items from the 2026-10-07 model-facing text audit: every string below is what the model reads, corrected to match what the engine actually does. English wording follows each surface's surrounding style; no claim was widened or narrowed beyond the code.

Six review fix waves ride on top: a 9-commit wave answering the first 12-lane review, a 6-commit wave (2026-11-02) answering the second 13-lane review — including one real regression the pinned canonical-action guard caught — a 7-commit wave (2026-11-04) answering a third from-scratch 14-lane re-review, a 7-commit wave (2026-11-04, round 4) answering a fourth from-scratch 14-lane re-review, a 6-commit wave (2026-11-04, round 5) answering a fifth from-scratch 16-lane re-review, and a 3-commit wave (2026-11-05, round 6) answering a sixth from-scratch 15-lane re-review, which also rebased the branch onto the pinvou3-clean tip that had moved seven commits ahead, and a 6-commit wave (2026-11-06, round 7) answering a seventh from-scratch 18-lane re-review (details below). Commit SHAs in the wave sections refer to the current post-rebase chain.

Item matrix

# Audit item Verdict Change
1 read description claimed text-only while the contract reader returns images fixed (numbers were already accurate) The audit-era "500 lines/16KB" figure belongs to the hidden legacy read_file compatibility tool — still 500 lines/16KB in file.rs, and not advertised in the model-visible tool catalog (it remains reachable by external MCP clients through the server's default file_read→read_file expose mapping; that legacy copy is recorded as a follow-up below) — it never described the visible read. The visible read's contract window defaults to 2000 lines/50KB (READ_MAX_LINES/READ_MAX_BYTES, file.rs), so the numbers in the description matched the code. The remaining inaccuracy — "Read a text file" while sniffed PNG/JPEG/GIF/WebP files return as inline image blocks and non-UTF-8 bytes decode lossily — is now disclosed, including the 5 MiB inline cap and BMP's omission-note behavior (fix wave). After the round-6 rebase moved the base onto the subagent-isolation work, which added a binary refusal before the lossy decode, the decode clause also carries the refusal exception: a payload with a NUL byte in its first 8 KiB, or beginning with a UTF-16 byte-order mark, is refused as binary (wave 6).
2 bash description hid fresh-shell and merged-stream semantics fixed Contract bash drains stdout+stderr through one shared pipe (shared_output_pipe) and concatenates them (finish_contract_bash_result), and each call spawns a fresh shell. Description now states both, with the redirect-to-file escape hatch. (The #126-era line was confirmed at shell.rs:4557 on the audit-era base; the line has since moved with base evolution.)
3 v4-best-practices taught hidden read_file/edit_file fixed Cites the visible read/edit tools. Extended the retired-name guard (PHANTOM_NAMES sweep + the read_file/exec_shell loop) to this asset and fixed the stale scope comment that documented the gap.
4 Run visible schema stripped verifier semantics fixed, then corrected commands now carries the program+args-not-a-shell clause and per-field descriptions; background now states task-id return, task/status tracking, and the wait-only-for-output/barriers rule. The first cut carried the hidden schema's Bash wording verbatim into the visible schema, which the pinned no_advertised_tool_teaches_a_retired_name guard rejects — the wave rewrote it to name task_shell_wait, the published waiter the background metadata itself advertises in poll_with. The second wave scoped the not-through-a-shell claim per execution path (background gates render through the platform shell) — see wave 2.
5a fim_edit first-occurrence anchors, no uniqueness check fixed (implementation) No check existed, so one was added: refusal when the prefix anchor matches more than once anywhere or the suffix anchor matches more than once after the prefix, naming the count; schema discloses the exactly-once contract. Three new tests (ambiguous prefix, ambiguous suffix, unique anchors reach the API boundary). The tool-level description now carries the same uniqueness clause as the schema (wave 2); the counting edges are pinned by tests (wave 4).
5b read_media 256 KiB default budget undocumented fixed Description and detail schema now give the ~256KB auto/low delivery budget vs the full 5 MiB budget kept by high/original and crop reads; docs/READ_MEDIA.md records the budget beside the other guards (fix wave). The result receipt's size line now says "(encoded payload)" — it prints the pre-base64 size, and the actual wire payload is the base64 form ~4/3 larger (wave 4).
5c tasks create authority flags bare "(action=create)"; gate_run BLOCKED undisclosed fixed allow_shell (shell permission; omitted resolves through the configured durable-task allow-shell setting, false unless enabled in config), trust_mode (relaxed workspace-boundary path validation, defaults false — production TaskManagerConfig::from_runtime hardcodes the default), auto_approve (auto-approved tool calls, full-access posture, defaults false per GHSA-72w5-pf8h-xfp4 opt-in). gate_run's command discloses the BLOCKED refusal for dangerous classifications unless the session auto-approves. The wave also discloses the nine remaining bare "(action=…)" properties (model, mode, timeout_ms, attempt bookkeeping, verification, attempt_id, limit) from their execution paths.
5d lsp line requirement for definition/references undisclosed fixed line description now names the operations that require it; pinned schema test updated.
5e 4 dead short-description frontmatter blocks fixed (deletion) Removed from fleet-manager, delegate, best-of-n, skill-creator. The line-based frontmatter parser never consumed them (no reader in any crate); names/descriptions/aliases-for untouched; catalog-matrix contract unaffected (name+invocation only). Generation bumped to 13 with the four previous-body digests recorded, so installed gen-12 copies refresh instead of fossilizing as user edits (fix wave); the four digests are anchored to embedded generation-12 fixtures (wave 4).
5f text.rs header listed CALM_PERSONALITY as assembly step 2 fixed (comment) Header now describes the real order — personality folded into the constitutional preamble — and marks the constant retired test-only text (tests assert Calm stays out of the default prompt). The wave finished the cleanup by dropping the equally-dead "Approval-policy overlays" layer and aligning the prompts.rs module header; the round-4 wave caught the three remaining stale comments, and the round-6 wave scoped the residual present-tense consolidation claim and fixed the header's zero-match test command (see wave 6).

No items were skipped: all audit targets still existed on the origin/pinvou3-clean base this audit ran against (f1eb10e at the time; the branch now sits on the further tip 11cba40, with the audit surfaces re-verified there in round 6) and are addressed above. Item 1's number half is recorded as already-accurate rather than fixed, with the code evidence in the table.

First review fix wave (2026-11-02)

From the fresh 12-lane review of this PR; every wave change was verified against the code first:

  • bfa732d91 — retitle of the head commit: it implemented item 5d (lsp.rs) but carried bca4382c9's docs(prompts) subject verbatim; now fix(tools): disclose lsp line requirement.
  • ca2af5ff6 — allow_shell copy correction: omitting the flag resolves through the durable-task profile snapshot (config.allow_shell(), default false), not the interactive session's effective setting (interactive_allow_shell(), default true); the first cut's "inherit the current session's allow-shell setting" was wrong under default config. Plus the nine remaining bare "(action=…)" schema fields disclosed.
  • 4447e58b8 — v4 retirement repair: retire_unchanged_v4_best_practices compared against the current body only, so item 3's edit stopped retiring exactly the installs that still teach retired names. The previously shipped body is now pinned and retired byte-exactly; the retirement test writes the legacy bytes (it previously wrote the same constant the retire function compares against, making its assertion a tautology), and an appended-line case pins the exact-match boundary.
  • 2ad8e553a — non-destructive upgrade half for 5e: generation 12 → 13 plus the four previous-body SHA-256 digests, per the SUPERSEDED_SHIPPED_BODY_DIGESTS convention (bump and entries together, as 10→11 and 11→12 did); catalog matrix fixture generation follows.
  • 4499d3983 — finishes 5f: drops the dead "Approval-policy overlays" layer from the text.rs header (the overlay constants were removed as dead code; approval policy travels as request-time runtime metadata) and corrects the prompts.rs module header that still named a personality overlay step.
  • 1a190462f — read description now discloses the 5 MiB inline cap and BMP's omission-note behavior (BMP is sniffed into the image branch and rejected by prepare_tool_image_bytes, so its bytes reach neither image nor text output).
  • 26ea31623 — validate_shell_working_dir no longer tells the model to pass a parameter "to exec_shell" (retired in v0.9.3) and drops the hidden-tool-only working_dir alias from the hint; asks for an explicit cwd.
  • e454f70eb — docs/READ_MEDIA.md delivery-budget bullet.
  • 969624f87 — Run schema regression fix described in matrix row 4.

Second review fix wave (2026-11-02, round 2)

From the fresh 13-lane re-review at 969624f87 (rebase re-verified as a no-op at the time); one MAJOR plus minors, each fixed against code evidence:

  • 5f86b1af6 — Run verifier copy scoped per execution path (review MAJOR): "Commands run directly as program+args, not through a shell" was false for background=true — only foreground gates spawn directly (Command::new(program).args(...)); background gates render the command into a single line (render_gate_command) and hand it to the detected platform shell via ShellManager, where POSIX quoting can fail under PowerShell (the repo's own background-gate test documents exactly that). The copy now scopes the claim per path. Also: background no longer names a literal task_ids field (that array exists only in tool-result metadata the model never sees — the visible payload carries per-gate task ids), and the commands[] items object now carries additionalProperties: false so the advertised schema matches CustomVerifierInput's deny_unknown_fields hard fail on unknown item keys.
  • 6521c3418 — bash working-dir hint made actionable: the reworded hints said "pass an explicit cwd", but the visible bash schema accepts only command/timeout/sandbox_permissions/justification (contract_bash_legacy_input rejects the rest), so the suggestion failed with "unexpected bash parameter(s)" on the very surface that surfaced the error — and the call is refused before spawn, so cd inside the command cannot help either. Surfaces that do accept cwd advertise it in their own schema, so the hint now names only recovery options that work everywhere (restore/remap, resume/fork).
  • 6935e8e12 — task mode approval claim corrected: "'plan' … and approvals stay enabled" was false for the reachable plan+auto_approve=true path — RuntimePolicyProjection::from_request maps auto_approve to ApprovalMode::Bypass regardless of mode, while plan's hard blocks (shell, writes, sandbox) hold. Reworded to say exactly that. Range restatements duplicating schema-declared minimum/maximum/default in the same property (timeout_ms range, limit default+range, attempt_index/attempt_count range) folded to one clamp clause (corrected to its exact type-error boundary in wave 4); defaults the schema does not carry ("Defaults to 120000/1") kept.
  • 6ee7116dd — fim tool-level description() now carries the uniqueness clause (sibling edit discloses at both levels); in-code comment tense fixed; test helper refused renamed to execution_error (the unique-anchors test reused it where nothing is refused).
  • 5c012817f — finishes the header cleanup: BASE_PROMPT's doc no longer claims runtime "mode / approval / … / compaction" layers (contradicting the corrected module headers); the volatile-content-last doc lists the real sequence (composed constitution prefix → WorldState fragments → authority-recap/locale-closer trailers, with the compaction relay explained as a user-message template); remaining bare "overlay" wordings (CALM_PERSONALITY section/doc, StaticPromptCtx fields, embedder hook) aligned with the retired framing.
  • 52d999609 — v4 legacy pin hardened to digest + fixture (review m-6): the full-body source constant was both the compare target and the test fixture, so transcription drift on either side would keep every test green while real legacy installs silently stop retiring. The pin is now a SHA-256 digest (the SUPERSEDED_SHIPPED_BODY_DIGESTS mechanism) and the historic bytes moved to a test fixture that asserts every run that they still hash to the pinned digest — drift on either side fails the suite (both directions mutation-proven, see Verification). Also restores dedicated coverage for the current-body retire arm (upgrade_retires_byte_exact_current_v4_body_installs) and aligns two comments with actual behavior (byte-exact keeps are retired at the next launch; feishu has no install path at all).

Note: wave 2 was originally pushed via the Git Data API during a git-endpoint outage, so at the time its server-side SHAs differed from the locally verified commits while carrying identical trees, messages, authors, and dates; the wave-3 push reconciled the branch, and from 2026-11-04 until the round-6 rebase the server-side history was byte-identical to the locally verified commits. The round-6 rebase (2026-11-05) rewrote every SHA again onto the moved base — the current chain is the locally verified one.

Third review fix wave (2026-11-04, round 3)

From a fresh from-scratch 14-lane re-review at 52d999609 (rebase re-verified as a no-op at the time — the branch then sat exactly on the pinvou3-clean tip, which is ahead of main in this fork). One blocker plus follow-ups, each fixed against code evidence:

  • ba047292d — shell working-dir recovery advice made true on every surface (round-3 blocker): "resume/fork the session from an existing workspace" was false — resume re-anchors to the same saved (missing) directory on every surface (TUI restore and headless resume both run the saved pair, so the advice looped), a CLI fork copies the saved workspace, and "remap" named no engine facility. The advice now splits on the same source flag as the label: recreate-or-switch for the saved session workspace (naming /workspace <path> and /fork after switching — the real command names; /cd does not exist), and recreate-or-use an existing directory for a requested working directory (the surfaces whose schemas advertise cwd, such as the task and verifier gates). A new test pins the not-a-directory branch and the absence of the false resume/fork advice. (Round-6 note: tool callers always resolve a concrete working directory, so today the rendered arm is the requested-directory one; the saved-workspace arm is truthful but currently reachable only from direct shell-manager callers — disclosed below.)
  • 6c6937860 — trust_mode's second effect disclosed: under auto_approve, a tool call the sandbox denies is retried with full access instead of being denied (approval_decision maps trust_mode to RetryWithFullAccess). One-line schema addition completing the interplay disclosures alongside the mode row.
  • fc416b7c4 — background gates return a task id per gate, null when a gate is skipped or fails to start; the copy no longer implies every gate gets an id.
  • 36636d20c — the hidden run_verifiers schema's unscoped "not through a shell" sentence now matches the visible Run schema's per-path wording (background gates render through the detected platform shell). Its background description is unchanged — task_ids and the Bash action-wait wording are accurate for that surface's metadata-level consumers.
  • 95fbd6ebc — read_media budget copy reframed as a budget ("fit a ~256KB delivery budget"), matching the low clause and docs; small images delivering far less than 256KB are no longer implied to be atypical.
  • 1eb944fb1 — new test pins the crop carve-out (crop+auto delivering past the ~256KB detail budget within the 5 MiB wire budget); mutation-proven RED when the crop arm is inverted, GREEN restored.
  • bf1ab736c — prompts docs: the volatile-content-last fragment list now names the continual-harness block and the imported project-instructions fragment the builder really adds, and the module header no longer claims every compile-time constant lives in the text module (authority recap and locale bookends live in prompts.rs).

Also disclosed for completeness (round-3 finding, no code change): empty fim anchors — accepted at base and edited at the first char boundary — are now refused with their boundary count ("matches N locations"), consistent with the exactly-once contract; the count is non-overlapping, so "locations" means non-overlapping sites, not start positions. Two corners pass through with count 1 and keep the base behavior: an empty suffix after a prefix ending at EOF, and an empty prefix on an empty file (round-4 review; now pinned by tests — the wave-4 note had claimed this pinning, but the tests only landed in wave 5).

Fourth review fix wave (2026-11-04, round 4)

From a fourth from-scratch 14-lane re-review at bf1ab736c (rebase re-verified as a no-op at the time; zero P0/P1 across the review, fork-guard registry fully intact). Body corrections plus residual-copy and pin fixes, each verified against code evidence:

  • c27064ace — "out-of-range values are clamped, not rejected" was false for negative integers: the u64 parser rejects them as a type mismatch (as_u64, type_mismatch) and they never reach a clamp. Per-field boundary copy instead (clamped into the declared range; below-1 clamp for the attempt fields; the negative/non-integer rejection named), plus pins for the corrected sentences and for the reviewed authority claims (plan blocks not lifted by auto_approve, durable-task allow-shell fallback, trust_mode full-access retry, BLOCKED gate refusal) that had no schema pin.
  • da8a0276f — the read_media result receipt printed the pre-base64 encoded size as "(wire payload)"; the actual wire payload is the base64 form (~4/3 larger, as the image-attach code's base64 note and docs/READ_MEDIA.md's wire-size bullet describe). Now "(encoded payload)".
  • 0b09401aa — three header comments survived the assembly-layer cleanup: the prompts.rs section header still said "every bundled prompt layer lives in text.rs" (the authority recap and locale bookends are defined in prompts.rs), the handoff doc still said the relay artifact is "prepended" to the system prompt (it is routed as the late WorldState token-budget/continuity fragment), and the environment-block doc still claimed placement "before configured instructions / skills" (it leads the workspace WorldState fragment, assembled after the constitution prefix). The text.rs module header aligned with the corrected prompts.rs header.
  • bcb0dae84 — fim counting edges pinned: an empty prefix is refused with the char-boundary count (characters + 1, not bytes + 1 — the multi-byte fixture discriminates a byte-offset refactor); suffix occurrences before the prefix are invisible to the uniqueness check; a suffix that only occurs before the prefix stays SuffixNotFound.
  • 8d0e997cb — background task-id null arms pinned through start_background_gates: a gate without a program reports skipped with its reason, and a gate whose working directory does not exist fails shell-manager validation and reports failed_to_start with the accurate error.
  • bfee93377 — the load-bearing Run/bash/read copy sentences pinned against silent reverts: the per-path shell scoping (foreground direct spawn vs. background render through the detected platform shell, POSIX-quoting-under-PowerShell caveat), the task-id-null clause (and the absence of a literal task_ids field in the visible copy), the bash merged-stream/fresh-shell claims (and the absence of a separate-stream promise), and the read image/BMP/UTF-8 disclosures (and the absence of a text-only claim).
  • 5add71c4a — the four gen-13 superseded digests anchored to embedded generation-12 fixtures, hashed against their registered entries on every run (the existing shape test only rejects digest == current, so a single wrong hex digit would have silently fossilized real generation-12 installs); the behavioral supersede check rides along.

Also disclosed for completeness (round-4 findings, no code change):

  • The tasks mode property advertises a closed enum (agent/plan/operate), but execution passes the string through to RuntimePolicyProjection::from_request, which still accepts the legacy aliases (yolo/4/bypass/bypass-permissions/bypasspermissions) and maps them to ApprovalMode::Bypass even when auto_approve is false or omitted; unknown modes fail closed. Enforcing the advertised enum (or disclosing the aliases in the schema) is a behavioral decision, separate follow-up (runtime_policy.rs).
  • pr_attempt_record values above u32::MAX for attempt_index/attempt_count wrap through as u32 after the below-1 clamp (e.g. 4294967296 records as 0, violating the advertised minimum); absurd-input hardening, separate follow-up (tasks.rs).
  • The copy pins in this wave are direct equality/contains assertions against live schema and description strings; the round-2 mutation re-proofs below are unchanged.

Fifth review fix wave (2026-11-04, round 5)

From a fifth from-scratch 16-lane re-review at 5add71c4a (rebase re-verified as a no-op at the time — the base tip was still the merge base; zero P0/P1 across the review; smuggling verdict CLEAN — every hunk of the then-38 commits attributed, linear history, one topic each; performance NEUTRAL — per-turn catalog strings are memoized per registry build, the fim scans run before a network call, and the startup digest hashing is a one-time microsecond cost). Three P2s and three in-theme residuals fixed, each verified against code evidence:

  • 33395ff9d — the model-visible harness tool no longer teaches the hidden rlm surface: the description's "Keep large source material in rlm" clause and the overview result's rlm runtime-context line named a compatibility-only tool (model_visible() == false, and its own description says not to teach it to new model turns) — the exact phantom class item 3 fixed in the v4 skill and that the PHANTOM_NAMES sweep guards; allowlist-deriving hosts reject the very call the copy recommended. The copy now names the visible bounded-reread surfaces (read with offset/limit, retrieve_tool_result). (The removal went in unpinned; the round-6 wave adds the pin — see wave 6.)
  • d6d957228 — the fim "exactly once" copy now discloses the non-overlapping counting rule ("counted without overlap" in the tool description and both anchor schema descriptions), matching the sibling edit tool's "unique, non-overlapping region" wording; a new copy test pins all three sentences. (Formerly the disclosed out-of-scope bullet below — now fixed.)
  • 279b313c3 — the two fim pass-through corners are now pinned by tests: an empty prefix on an empty file proceeds to the API boundary (count 1 at the single char boundary) and an empty suffix after a prefix ending at EOF proceeds (count 1 in the empty region). The round-4 note had claimed this pinning existed; it did not — the tests land here.
  • 3f07d01cb — the read_media receipt's "(encoded payload)" label is pinned, with the absence of "(wire payload)" asserted — the one load-bearing round-4 copy change no test guarded.
  • 8b6d17e0f — allow_shell's "Passing true or false is always honored" absolute is scoped to the mode ("honored subject to the mode — plan keeps shell disabled even when this is true"), matching the mode property's own precedence disclosure; both sentences pinned in the authority-copy test.
  • 71b61ea5d — prompts.rs: the module header no longer ends on the stale "Keeping every layer's text in one module makes prompt tuning a single-file operation." sentence that contradicted the corrected constant-split header directly above it, and the handoff test whose name still said "prepended" (the doc saying that was corrected in wave 4; the test asserts containment only) is renamed to what it actually pins.

Also disclosed for completeness (round-5 finding, no code change): the skills refresh path treats an unreadable or non-UTF-8 installed SKILL.md as empty (read_to_string(...).unwrap_or_default()), so such a file is refreshed without the superseded-digest user-edit check — pre-existing install_one behavior exercised by every generation bump, adjacent hardening, separate follow-up.

Sixth review fix wave (2026-11-05, round 6)

From a sixth from-scratch 15-lane re-review (smuggling CLEAN — every hunk of all 44 then-commits attributed across the 27-file delta; body-truth audit; commit-hygiene audit; base-interaction audit; mutation suite; performance re-measurement), run after rebasing the branch onto the pinvou3-clean tip 11cba403d, which had moved seven commits ahead (subagent claim/worktree isolation #123/#124, run-completion backport #129, guardrail prompts #131, fetch_url copy drop #133, locale-pack mode-copy drop #134, dependency bumps #135). The rebase replayed all 44 commits with zero conflicts, and the moved head was verified to differ from the pre-rebase head by exactly the base delta (line-set identity). The moved base both answered prior interaction questions (the new shell kill-path, authority-transition notice, fetch_url and locale copy drops do not contradict any corrected copy; guardrail arming was checked against the unarmed default and its armed-only exception was missed — corrected in wave 7) and introduced one real contradiction on this PR's own surface, fixed with two residuals:

  • 4a36f4be6 — read decode clause scoped to the new binary refusal: the subagent-isolation work added contract_read_payload_is_binary (a NUL byte anywhere in the first 8 KiB sniff window, or a leading UTF-16 byte-order mark → refused with extractor suggestions before any decoding), which made this PR's own "bytes that are not valid UTF-8 become replacement characters instead of failing the read" absolute false at the new tip — stray high bytes and NUL-free heads still decode lossily; the refused classes do not. The clause now carries the refusal exception; the description pin asserts the exception and forbids the unscoped sentence end from returning; and the refusal message's own "read returns text only" absolute (the same tool returns inline images for sniffed formats) is corrected to "read returns text or inline images, never raw binary", pinned in the behavioral refusal test. All three changes mutation-proven RED/GREEN.
  • bcf944c59 — the wave-5 harness fix is now pinned: the rlm removal landed with copy only, and the round-6 mutation suite showed re-adding both original rlm mentions passed every guard and suite — the one unpinned copy fix in the PR. A new test pins the absence of rlm and the presence of the visible bounded-reread surfaces (read with offset/limit, retrieve_tool_result) in both model-visible strings (the description and the overview result's runtime context); mutation-proven RED when either mention returns.
  • 5fbf16822 — two header residuals the cleanup missed: the consolidation sentence still claimed in the present tense that the whole prompt contract reads top-to-bottom in this one module, contradicting the corrected split header directly above it (the authority recap and locale bookends never lived here — the claim is now scoped to the constants that moved); and the header's edit-loop command ran cargo test -p codewhale-tui --bin codewhale-tui prompts, which matches zero tests (the prompts tests live in the lib target) — it names --lib now.

All 53 commits carry DCO Signed-off-by, one per topic.

Seventh review fix wave (2026-11-06, round 7)

From a seventh from-scratch 18-lane re-review — per-surface truth lanes for read/bash/fim/Run/tasks/skills/prompts, a full hunk-by-hunk commit attribution audit, a body-truth audit (all 41 cited SHAs, all counts, the recovered pre-rebase chain and its line-set identity), an independent base-interaction audit, 16 mutation probes, a full 12,448-test suite run, and fork-guard — at the unchanged pinvou3-clean tip 11cba403d (the rebase check was a verified no-op; CI green). The review confirmed every earlier wave's fixes and found one real contradiction, two pin gaps, and two schema residues, each fixed against code evidence:

  • a326ba9ce — the auto-approve copy now carries the armed-guardrail exception (round-7 blocker): base feat(engine): host-armed guardrail prompts + authority-transition notice #131's host-armed ask_on_guardrail consent path changed the approval pump to auto_approve && !approval_force_prompt (runtime_threads.rs:9498, pinned by the base's forkguard_pump_never_auto_approves_a_force_prompt_hold), so on a host that arms guardrail prompts, guardrail-class calls (the non-bypassable tool set, typed ask rules, repo-law holds, sandbox escalation) force a real human approval even for an auto_approve=true task — falsifying this PR's own "no approval prompts" absolute and the mode row's "which otherwise skips approval prompts" parenthetical (both written 2026-11-02, three days before feat(engine): host-armed guardrail prompts + authority-transition notice #131 landed). The round-6 audit had listed "guardrail arming" among base changes that "do not contradict any corrected copy" — that check was answered for the unarmed default and missed the armed-only exception, so the fix lands in this PR rather than a disclosure, per the wave-6 precedent. The mode row also now discloses that trust_mode's effects still apply in plan (the sandbox-retry path has no mode re-check — the "'plan' runs read-only" corner), and the pins assert both exception clauses while forbidding the unscoped absolute from returning (mutation-proven RED).
  • 4f00c64fe — the tasks description's "Use task_shell_start for long-running shell work." pointer is scoped to the surfaces that register the tool (allow_shell on, non-plan mode — the registry gates the runtime task shell tools behind ShellPolicy::Full), and the task_id property no longer lists read among the actions requiring an id (only cancel does; read and pr_attempt_* resolve through the same active-task default). Both pinned, mutation-proven.
  • 7936c7eff — the read_media delivery-budget sentences (the direct item-5b fix) are pinned against silent reverts; the round-7 mutation suite showed a probe flipping "~256KB" to "~2MB" passed the whole suite before this pin.
  • 8374e64c8 — a behavioral pin for the fim non-overlapping counting rule: a self-overlapping prefix ("aa" in "aaa") has exactly one non-overlapping site and reaches the API boundary; an overlapping counter would refuse (mutation-proven RED via the same guard).
  • 64202f1d9 — the shell working-dir advice pin is hardened: the requested-directory arm (the model-reachable one) now forbids resume/fork wording outright, and the saved-workspace not-a-directory branch gets its own test. Mutation-proven RED for a paraphrased revival of the false advice.
  • eab45d4aa — two constant-doc claims the header cleanup missed: BASE_PROMPT's doc summary listed sections the constant does not have (output format, toolbox reference, "When NOT to use", sub-agent sentinel protocol live in other layers), and the AUTHORITY_RECAP doc said it is "appended at the end of the system prompt" when the locale closer follows it for non-English locales (it is a per-request trailer after the WorldState fragments).

Also reviewed and left unchanged: the trust_mode sentence's deny-retry face is a different flow from the armed approval-card face (the SandboxDenied retry path does not consult arming), so its wording stands; the run/verifier hidden-schema per-path wording matches the visible copy in substance though it omits the PowerShell parenthetical; and the harness rlm, digest-fixture, and bash/read pins all held under the fresh mutation probes (13/16 RED; the three GREEN gaps are the ones this wave closes).

Deliberately out of scope (disclosed, not changed)

  • Audit item 16 also noted args/all_features hard-fail under action=verifiers (deny_unknown_fields). The visible schema already carries the two load-bearing semantic clauses — "(action=tests; rejected by action=verifiers)" on both descriptions, landed via fix(tui): align model-facing text with actual behavior #112 and pre-existing on this base; the advertise-then-reject shape of those two test-only params is a behavioral question left for a separate decision.
  • Legacy TaskRecord restore defaults a pre-field record's auto_approve to true (default_auto_approve, task_manager.rs:3105) — a fail-open restore path adjacent to the GHSA narrative; behavioral decision, separate follow-up.
  • The tasks mode enum is advertised but not enforced (legacy aliases escalate to Bypass; see the fourth-wave disclosures above) — behavioral follow-up. The gate property is the same advertise-not-enforced class: any string is accepted and only clippy/test get dedicated failure classification — same follow-up batch.
  • The skills refresh path's empty/unreadable-file conflation above is the same class as this list's other adjacent-hardening entries: behavioral, needs its own decision.
  • Fourteen bare "(action=…)"-style property descriptions remain in other tool schemas — ten of them model-visible (automation.rs:149,153,157,174,183; github/schema.rs:26,30,35,56,68(+64 with copy)) and four in the hidden rlm compatibility schema (rlm.rs:283,287,293,297, model_visible() == false). Same copy class as item 5c, but each needs its execution path traced before truthful copy can be written — follow-up batch, not risked here.
  • Foreground run_gate misreports a nonexistent gate cwd as "{program} is not installed or not in PATH" (verifier.rs:1167-1181) while the background path errors accurately — pre-existing behavioral asymmetry, adjacent to the Run schema copy; separate behavioral fix.
  • The user-facing docs/HOOKS.md still cites exec_shell extensively as a live tool name; that is user documentation, not model-facing copy — separate docs follow-up.
  • Round-6 base-interaction residues, disclosed not changed (each needs its recovery path traced before truthful copy can be written): base-added subagent refusal messages advise "cancel it with exec_shell_cancel" (subagent/mod.rs write-scope contention and bounded-file-target refusals) — no tool is registered under that name (it is a canonical-action label), and the engine's own unknown-tool guidance points at tool_search instead; task_shell_start's relayed background results and the foreground timeout-recovery metadata name Bash/hidden-waiter recovery paths on surfaces whose published waiter is task_shell_wait; the hidden verifier level description's quick/full split is inverted for Go (quick already runs go test ./... — add_go_gates adds the test gate at every level); and the MCP relay's default expose serves the legacy read_file/write_file copy to external clients (item 1's matrix note) — legacy-copy class, separate decision.
  • BMP detection is a two-byte "BM" prefix heuristic (primitive_image_mime): a text file beginning with the literal bytes "BM" is routed to the image arm and returned as an omission note. Pre-existing sniff behavior, adjacent to item 1's BMP clause.
  • The retired v4-best-practices example still shows model="deepseek-v4-flash" (parse-accepted but unadvertised on the agent schema), type="verifier" (a legacy alias; the advertised enum spells it test), and "(thinking off)" prose the example call does not realize — rewording the body would require a fresh digest pin for the intermediate shipped bytes; follow-up.
  • The shell working-dir advice's saved-session-workspace arm (/workspace//fork suggestions) is currently unreachable from model-facing tool paths: every tool caller resolves a concrete working directory (an omitted cwd falls back to the session workspace, which then validates as a requested directory — truthful and actionable, but the /workspace / /fork advice renders only for direct shell-manager callers today). The false round-2 advice is gone from every arm; the reachability asymmetry is a behavioral follow-up.
  • allow_shell's durable-task fallback resolves against the TaskManager's startup-time configuration snapshot: a mid-session config change reaches newly created tasks only after a restart. The schema copy describes the mechanism, not the snapshot freshness; freshness disclosure is a follow-up.
  • fim max_tokens truncates through as u32 (4294967296 reaches the API as 0) — same absurd-input hardening class as the disclosed attempt-field wrap, separate follow-up.

Verification

  • cargo fmt --all -- --check: clean (re-run after the sixth wave).
  • Touched-module filter (tasks, fim, verifier, run_tool, file_tool, read_media, skills, prompts, harness): 449 passed / 0 failed post-wave-6 at head 5fbf16822, including the round-6 read-refusal pin and the harness rlm pin. (The 719 figure quoted in earlier rounds was recorded at the pre-rebase base with a different filter composition; module-filter counts drift with the base's own test population — zero failures at every round.)
  • cargo test -p codewhale-tui --lib --locked -- --test-threads=1 forkguard: 285 passed / 0 failed post-round-6 (up from 240 at the previous base entirely via base-inherited tests; this branch adds no forkguard tests). At default parallelism the forkguard filter currently deadlocks on the reference machine — an ABBA ordering between the tests' shared test_env_lock and engine-state locks, inherited from base-side commits, reproducible independently of this branch; single-threaded is how fork-guard.sh itself runs this lane, and it is green there.
  • The pinned no_advertised_tool_teaches_a_retired_name guard is green (it lives in tools::canonical_action, not under the forkguard name filter).
  • cargo clippy -p codewhale-tui --lib --all-features --locked: byte-identical between base 11cba403d and head under local clippy 1.99.1 — eight pre-existing diagnostics (six clippy::too_many_arguments, two clippy::collapsible_if), none introduced or touched by any wave. (The "six diagnostics" figure in earlier rounds was recorded against the previous base f1eb10e and is base-dependent; the exact command exits 101 on both trees — a pre-existing workspace lint deny, unchanged by this branch.)
  • Full-suite note: on the reference machine the full lib suite currently aborts with a stack overflow in runtime_api::tests::events_endpoint_respects_since_seq_cursor before reaching a tally; the identical abort reproduces standalone on a clean checkout of the base, so it is machine-specific and not introduced by this branch (fork CI, which runs the full gate, is green on this branch). The earlier post-wave-2 tally — 12,292 tests, 12,275 passed / 4 failed / 13 ignored, the four failures being machine-specific load-order flakes (remote_control ×3, client::operation_tests ×1) that pass in isolation — was recorded on this machine before that abort appeared.
  • Mutation re-proofs: the wave-2 digest pin survives — deleting the legacy-arm retire condition turns upgrade_from_v4_installs_pack_and_retires_unchanged_v4_best_practices RED, and a one-byte fixture edit turns the same test RED at the pinned-digest self-check. The round-2 review's 9-mutation suite (fim refusal trio, retirement repair, phantom-name sweep, canonical Run-schema retired-name guard, lsp pin) was RED/GREEN as designed. The round-6 review ran a 13-probe suite over the pre-wave-6 head: 12 RED as designed and one GREEN — the unpinned wave-5 harness rlm removal, which the wave-6 pin now closes (re-proven RED after the wave).
  • Fork-guard: no registered fingerprint is broken by this branch — verified across the full 302-needle registry (216 CodeWhale-side) at both base 11cba403d and head: the 27 registered needles touching files this branch modifies are all present at head, and the one needle absent from its registered file at head (complete returned tool result at, registered on core/engine/context.rs) is an inherited base-side relocation — base-side commit fix(subagent): 固定任务身份与完整输出恢复 #124 moved the string to tools/truncate.rs, and it is absent from the new base as well, so it is a parent-repo registry re-point, not a branch regression. The forkguard_* identifier floor (64) holds at 301 at head (255 at the previous base; the growth is entirely base-inherited).
  • CI: the fork CI lane (check, gate, Gitleaks, Check Signed-off-by, Type Check, credentials, CodeQL analysis ×4, link) runs green on this branch at head; the earliest waves predate the maintainer approval that unblocked workflow runs for this author.

No-Issue: issues are disabled on this repository; audit-batch corrections to already-shipped model-facing text, tracked through the review rounds in this PR.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fresh 12-lane re-review at 56105b1 (unchanged during review; base pinvou3-clean f1eb10e is the branch tip, so the PR is already exactly on base — no rebase needed). Every claim below was verified first-hand against the code; local gates were run on this machine (rustc/cargo 1.99.1) because the PR currently shows no CI runs.

Verdict: CHANGES_REQUESTED

No blockers, no smuggling, no incorrect behavioral code. The behavioral change (item 5a) is sound and well-tested. Two MAJORs block: one commit-metadata defect, one copy-vs-code divergence in the PR's own added sentence — the exact class this PR exists to eliminate.

Item matrix verification

Item Copy-vs-code claims Verdict
1 read 2000/50KB matches READ_MAX_LINES/READ_MAX_BYTES (file.rs:523-524) enforced for this exact tool; image sniffing magic-byte-based, exactly PNG/JPEG/GIF/WebP; lossy UTF-8 decode (file.rs:768-770); BMP correctly unclaimed (sniffed into image branch at file.rs:643, rejected at image_attach.rs:144) ACCURATE
2 bash Merged stream applies to ALL invocations (contract path unconditional, one shared pipe, stderr structurally empty → finish_contract_bash_result concatenation is arrival-ordered); fresh shell per call, cwd fixed at manager construction, no cd/env persistence; unchanged rest spot-verified (caps 2000/50KB at output.rs:8-9, 120s default at shell.rs:4452, $SHELL detection) ACCURATE
3 v4 skill V4_BEST_PRACTICES_BODY is include_str! of the shipped asset (byte-identical, verified); read/edit/grep_files live and model-visible; zero phantom/retired names remain anywhere in assets; scope comments (v4 swept, feishu opt-in unswept) verified ACCURATE
4 Run schema Foreground gates direct Command::new(program)+args, no shell; background returns task_ids immediately, task_status completion surface; wait/task_shell_wait framing matches codebase language; every visible clause has a hidden-schema counterpart, no semantic drift ACCURATE
5a fim refusal Correct placement (after NotFound, before API call and write, side-effect-free); UTF-8 boundary-safe; locate/count semantics identical and match the new schema wording; all 3 tests non-vacuous (client check is at fim.rs:178-190, after validation); old first-occurrence behavior confirmed at base; no prompt/skill/doc teaches the old semantics SOUND
5b read_media 256KiB budget constant + encode ladder; crop short-circuits the detail budget (read_media.rs:682-686) so the blanket "crop reads keep the full 5 MiB" is exactly right, including crop+auto; 1024/2048/4096px caps verified; auto default verified ACCURATE
5c tasks trust_mode semantics + from_runtime hardcode false (task_manager.rs:731) across both production paths; auto_approve default false, ApprovalMode::Bypass→full_access, no prompts, GHSA cited in code; gate_run BLOCKED-before-spawn verified mostly ACCURATE — see M-2 for allow_shell
5d lsp Exactly diagnostics/read_lints/symbols/definition/references exist; only definition (mod.rs:513) and references (mod.rs:530) hard-require line; pinned test updated, no stale pins repo-wide ACCURATE
5e frontmatter Parser (skills/mod.rs:596-787) consumes only name/description/invocation/aliases-for/description_<tag>; removal is a parse no-op; base-tree sweep was exactly the 4 files; YAML/aliases-for/catalog-matrix intact ACCURATE — see m-2 for digest convention
5f text.rs Personality genuinely folded into constitution (prompts.rs:1043-1045; tone text inside BASE_PROMPT); CALM_PERSONALITY zero non-test references; guard tests exist as described; old header factually wrong (15e4134 already deleted the overlay composition) ACCURATE — see m-3

MAJOR

M-1 — Head commit 56105b1 message describes the wrong change. Its subject is byte-identical to 9fbce4d's ("docs(prompts): fix stale overlay doc comment") but it actually implements item 5d: the lsp.rs line schema description plus the pinned-test update. The commit body is accurate and the content is 100% in scope (test co-committed), but a docs(prompts) title on a fix(tools) schema change breaks bisectability and the sibling pattern. Please retitle (e.g. fix(tools): disclose lsp line requirement) before merge.

M-2 — New allow_shell copy diverges from actual inheritance (tasks.rs:198). "Omit to inherit the current session's allow-shell setting" — actual resolution is req.allow_shell.unwrap_or(self.cfg.allow_shell) (task_manager.rs:1683), where cfg.allow_shell is the durable-task profile snapshot from TaskManagerConfig::from_runtime → config.allow_shell() (task_manager.rs:730, config.rs:7363, default false). The interactive session's effective setting is a different accessor, interactive_allow_shell() (config.rs:7377-7378, default true). Under default config, an interactive session that omits the field produces a task with shell disabled while the copy implies it inherits the session's enabled state. Fail-closed direction, so behavior is safe — but this is precisely the copy-widening this PR declares eliminated ("no claim was widened or narrowed beyond the code"). Suggested one-line reword: "Omit to use the session's durable-task allow-shell default (false unless explicitly enabled)." No pinned test breaks (tasks:: targeted tests 15/15 pass and will re-verify).

MINOR

  1. v4 retirement test is now tautological. upgrade_from_v4_installs_pack_and_retires_unchanged_v4_best_practices (system/tests.rs:557) writes V4_BEST_PRACTICES_BODY — now the post-PR body — so it no longer simulates any real install; a test pinning the actual pre-PR bytes and asserting retirement would now fail. The PR already discloses the underlying retirement neutralization; the offered "legacy-body second candidate" (pinned pre-PR byte constant) would fix both at once.
  2. Bundled-body change convention not followed. The 4 changed SKILL.md bodies are in BUNDLED_SKILLS, but SUPERSEDED_SHIPPED_BODY_DIGESTS got no entries and BUNDLED_SKILL_VERSION stays "12" (system.rs:25-32 convention; refresh only on bump at install_one). Installed gen-12 copies keep the dead lines and will be misclassified as user edits by the skills audit on a future bump. Parse-identical, cosmetic impact; append the 4 old-body digests with the next bump (old-body SHA-256 prefixes: best-of-n b72d093f, delegate 169640cc, fleet-manager fa4369c7, skill-creator 6fb9e921).
  3. Renumbered header still lists a nonexistent layer. text.rs:19 "2. Approval-policy overlays" — the overlay constants were deleted as dead code in 15e4134 (ancestor of base); approval policy now travels as runtime metadata and tests assert its absence from prompts (prompts.rs:3353-3356). The PR edited and renumbered this exact block; a 2-line follow-up closes it.
  4. Verification-section counts don't reproduce here. Stated filter set yields 258 passed / 0 failed (not 395) — substance confirmed: 3 new fim tests pass by name, full lib suite = base + 3, all failures pre-existing env flakiness on untouched files. Clippy head == base diagnostics, so no-new-diagnostics holds, but this environment shows 6 pre-existing diagnostics (5× too_many_arguments + the collapsible_if), not one. Gates overall: fmt clean, tests green on substance.
  5. Audit completeness rests on the PR's own matrix. No authoritative 2026-10-07 item list exists in either repo, history, or GitHub (only pointer: tests.rs:220 → merged #112, whose cited audit doc is also absent). All falsifiable per-item claims checked out; "No items were skipped" is unverifiable by design.
  6. read description omits the 5 MiB inline cap (pre-existing): >5MiB PNG/JPEG/GIF/WebP silently becomes an omission note (image_attach.rs:58,144); the sibling read_media description updated in this same PR discloses 5 MiB, which makes the omission stand out.
  7. Adjacent follow-up candidates (pre-existing, not this PR's scope): shell.rs:133 runtime error cites retired exec_shell; legacy TaskRecord restore defaults auto_approve to true (task_manager.rs:3105, serde fallback) — fail-open adjacent to the GHSA narrative; 9 remaining bare "(action=…)" descriptions in tasks.rs; docs/READ_MEDIA.md lacks the 256KB budget.

Process / gates

  • DCO verified 10/10; conventional format otherwise correct.
  • No CI runs on the PR: the author isn't in .github/APPROVED_CONTRIBUTORS, so fork-ci runs sit at action_required per the repo AGENTS.md — needs a maintainer run-approval or /lgtm.
  • No smuggling: git diff --name-only base..head is exactly the 14 item files; every hunk attributable; both out-of-scope disclosures verified TRUE (args/all_features deny_unknown_fields at verifier.rs:92-99; retire_unchanged_v4_best_practices byte-compare at system.rs:528-543, precedent b1fcde7 confirmed).
  • Parent-repo follow-up after the gitlink bump (not this PR's defect): fork-modifications.md T2 entry, fork-guard fim fingerprint + EXPECTED_HEAD/COMMITS advance (20 commits now unregistered; 17 registered fingerprints already stale at the current base), a forkguard_* behavior test for the new refusal, fork-guard.sh --fast. 0/196 registered fingerprints are broken by this PR. The fim refusal is also a reasonable upstream candidate per fork-policy §2.

Clean areas (verified explicitly)

Fim edge cases (empty anchors now refused consistently with the exactly-once contract; self-overlapping undercount is the only residual gap and is not a distinct edit site; AnchorsOverlap dead before and after — pre-existing), UTF-8 byte-boundary safety, refusal channel consistency, no external callers on the old semantics, no unbounded-read regression; bash unchanged claims; hidden-schema parity for Run; read_media crop/budget/dimension constants; lsp operation inventory; frontmatter parser and catalog contract; no contradictions with any other model-facing doc.

asto18089 added a commit that referenced this pull request Nov 2, 2026
In-range lock refresh closing all 4 open Dependabot alerts (4 high):

- undici 7.29.1 -> 7.30.1 via wrangler 4.150.0 -> 4.156.1 -> miniflare
  5.20261006.2-beta -> 5.20261101.0-beta in root, web/, and
  telemetry-ingest/: closes GHSA-xjj6-f33q-929c / CVE-2026-109869
  (alerts #139, #142, #145; fetch DoS via unhandled error on excessive
  content-encodings). This closes the follow-up PR #120 left open:
  wrangler 4.156.1 (latest stable) exact-pins miniflare
  5.20261101.0-beta (2026-11-01), which exact-pins undici 7.30.1, so a
  plain `npm update wrangler` per directory lands there. Collateral,
  all npm-canonical: workerd 1.20261006.1 -> 1.20261101.1 (+ its five
  platform binaries), tslib 2.8.1 -> 2.9.1 in root and
  telemetry-ingest, `libc` metadata added to the existing
  @img/sharp-* entries in root and telemetry-ingest, wrangler's
  @cloudflare/workers-types peer range following to ^5.20261101.1, and
  web's nested miniflare ws 8.21.0 hoisting to the top level
  (content-identical). No other packages move.
- @azure/msal-node 6.0.1 -> 7.1.2 in extensions/vscode/: closes
  GHSA-xcx6-jx8p-x3h5 / CVE-2026-109461 (alert #136; auth-code flow
  origin validation; first patched 7.1.0) via the native in-range
  path: @azure/identity 4.14.1 (2026-10-29) requires @azure/msal-node
  ^7.1.0 and satisfies @vscode/vsce's declared ^4.13.2, so
  `npm update @azure/identity` moves identity 4.14.0 -> 4.14.1 and
  msal-node -> 7.1.2 with no manifest change. msal-common follows
  upstream's native shape (top-level 16.14.2 for msal-browser 5.25.1,
  nested 16.15.0 under msal-node; both exact pins). msal-node 7.x
  keeps the confidential-client surface identity uses (jsonwebtoken
  ^9, node >= 20); the path is dev-only (vsce).

Verified:
- npm ci green in root, web/, telemetry-ingest/, extensions/vscode/
- telemetry-ingest: npm run typecheck, npm run check (wrangler 4.156.1
  deploy --dry-run), npm test (171 passed)
- web: npx wrangler --version -> 4.156.1, npm run cf-typegen, npm test
  (408 passed in 47 files)
- extensions/vscode: npm run check (tsc), vsce package
  --no-dependencies (valid 13-file vsix), and an identity -> msal-node
  7 live smoke: ClientSecretCredential.getToken() builds and parses a
  real AAD protocol response through msal-node 7.1.2
- npm audit: root and telemetry-ingest report 0 vulnerabilities; web
  reports 20 moderate advisories (glob via @opennextjs/cloudflare
  1.20.9, minimatch via the eslint toolchain) ingested after this
  branch's initial verification; all devDependencies, none open GitHub
  alerts, left for a separate decision

Out of scope, unchanged: npm audit reports follow-redirects (high) in
integrations/{feishu,wecom}-bridge and qs + minimatch (moderate) in
extensions/vscode; none of these are open GitHub Dependabot alerts, so
they are left for a separate decision.

Signed-off-by: asto <asto18089@126.com>
@asto18089
asto18089 force-pushed the fix/model-facing-copy-residue branch from 56105b1 to 9e24145 Compare November 2, 2026 09:15
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed (56105b1 → 9e24145, 8 commits) answering the 12-lane review above. Every finding was verified against the code before fixing:

MAJORs

  • M-1: head commit retitled to fix(tools): disclose lsp line requirement (was a byte-duplicate of 9fbce4d's subject on an lsp.rs change).
  • M-2: allow_shell copy now states the real resolution — omitted falls back to the configured durable-task allow-shell setting (default false), not the interactive session's effective default (true).

Beyond the two posted MAJORs, the wave closed the valuable minors and caught one regression neither the review nor the PR's local gates had caught:

  • The full-suite run tripped no_advertised_tool_teaches_a_retired_name: item 4's visible Run schema said "Bash with action 'wait'" — a hidden compat name the catalog can't return, and an action the visible bash tool doesn't take. Rewritten to name task_shell_wait (the published waiter the background metadata itself advertises in poll_with); 9e2414596.
  • v4 retirement neutralization: the previously shipped body is now pinned (V4_BEST_PRACTICES_LEGACY_BODY) and retired byte-exactly; the tautological retirement test now writes the legacy bytes, with an appended-line keep-case; 98c90cdae.
  • Non-destructive upgrade half for the four stripped bundled bodies: generation 12 → 13 + the four superseded digests + catalog-matrix fixture generation; af11d5744.
  • Item 5f finished: dead "Approval-policy overlays" layer dropped from the text.rs header; prompts.rs module header corrected; 29a1ecb76.
  • read description now discloses the 5 MiB inline cap and BMP's omission-note path; 0b0495998.
  • shell recovery hint no longer cites retired exec_shell or the hidden-only working_dir alias; 3568e488c.
  • The nine remaining bare "(action=…)" task fields disclosed from their execution paths; docs/READ_MEDIA.md records the 256 KiB budget; b80ce3e3f, b3f723e5d.

Gates (local, rustc/cargo 1.99.1): fmt clean; touched-module filters all 0-failed (incl. the new legacy-addendum test); full lib suite 12277 passed / 1 failed, the failure being the known load-order flake remote_control::separate_predispatch… that also fails on a clean f1eb10e and passes in isolation; clippy diagnostic set byte-identical to base (6 pre-existing, none new); no fork-guard fingerprint broken. PR body's Verification section was rewritten with the corrected counts (the earlier 395/single-diagnostic claims didn't reproduce).

Out-of-scope follow-ups disclosed in the body: legacy TaskRecord auto_approve-defaults-true restore path, and the fim self-overlapping-anchor residual.

Still needs a maintainer to approve the fork-ci runs (author runs sit at action_required).

asto18089 added a commit that referenced this pull request Nov 2, 2026
In-range lock refresh closing all 4 open Dependabot alerts (4 high):

- undici 7.29.1 -> 7.30.1 via wrangler 4.150.0 -> 4.156.1 -> miniflare
  5.20261006.2-beta -> 5.20261101.0-beta in root, web/, and
  telemetry-ingest/: closes GHSA-xjj6-f33q-929c / CVE-2026-109869
  (alerts #139, #142, #145; fetch DoS via unhandled error on excessive
  content-encodings). This closes the follow-up PR #120 left open:
  wrangler 4.156.1 (latest stable) exact-pins miniflare
  5.20261101.0-beta (2026-11-01), which exact-pins undici 7.30.1, so a
  plain `npm update wrangler` per directory lands there. Collateral,
  all npm-canonical: workerd 1.20261006.1 -> 1.20261101.1 (+ its five
  platform binaries), tslib 2.8.1 -> 2.9.1 in root and
  telemetry-ingest, `libc` metadata added to the existing
  @img/sharp-* entries in root and telemetry-ingest, wrangler's
  @cloudflare/workers-types peer range following to ^5.20261101.1,
  web's nested miniflare ws 8.21.0 hoisting to the top level
  (content-identical), and the nested miniflare/node_modules/undici
  copies in web/ and telemetry-ingest/ moving to a single top-level
  7.30.1 entry (root's undici was already top-level). No other package
  versions change.
- @azure/msal-node 6.0.1 -> 7.1.2 in extensions/vscode/: closes
  GHSA-xcx6-jx8p-x3h5 / CVE-2026-109461 (alert #136; auth-code flow
  origin validation; first patched 7.1.0) via the native in-range
  path: @azure/identity 4.14.1 (2026-10-29) requires @azure/msal-node
  ^7.1.0 and satisfies @vscode/vsce's declared ^4.13.2, so
  `npm update @azure/identity` moves identity 4.14.0 -> 4.14.1 and
  msal-node -> 7.1.2 with no manifest change. msal-common follows
  upstream's native shape (top-level 16.14.2 for msal-browser 5.25.1,
  nested 16.15.0 under msal-node; both exact pins). msal-node 7.x
  keeps the confidential-client surface identity uses (jsonwebtoken
  ^9, node >= 20); the path is dev-only (vsce).

Verified:
- npm ci green in root, web/, telemetry-ingest/, extensions/vscode/
- telemetry-ingest: npm run typecheck, npm run check (wrangler 4.156.1
  deploy --dry-run), npm test (171 passed)
- web: npx wrangler --version -> 4.156.1, npm run cf-typegen, npm test
  (408 passed in 47 files)
- extensions/vscode: npm run check (tsc), vsce package
  --no-dependencies (valid 13-file vsix), and an identity -> msal-node
  7 live smoke: ClientSecretCredential.getToken() builds and parses a
  real AAD protocol response through msal-node 7.1.2
- npm audit: root and telemetry-ingest report 0 vulnerabilities; web
  reports 20 moderate advisories (glob via @opennextjs/cloudflare
  1.20.9, minimatch via the eslint toolchain) ingested after this
  branch's initial verification; all devDependencies, none open GitHub
  alerts, left for a separate decision. No HIGH findings remain in any
  of the four refreshed manifests: the base audits carried 1 HIGH for
  undici in root, web/, and telemetry-ingest/, and 2 HIGH for
  msal-node in extensions/vscode (the advisory this bump closes)

Out of scope, unchanged: npm audit reports follow-redirects (high) in
integrations/{feishu,wecom}-bridge and qs + minimatch (moderate) in
extensions/vscode; none of these are open GitHub Dependabot alerts, so
they are left for a separate decision.

Signed-off-by: asto <asto18089@126.com>

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fresh re-review round at 9e24145 (unchanged during review; rebase re-verified as a no-op — merge-base == base tip f1eb10e, so the branch sits exactly on latest pinvou3-clean). 13 lanes this round: smuggling/attribution, fim deep-dive, Run-schema truth, tasks truth (all 13 rewritten fields traced to execution paths), shell/read/read_media truth, skills-system mechanics, assets+guard tests, prompts headers, body/DCO/disclosure audit, design, plus fresh local gates (fmt, clippy vs base, full suite, 9 mutations). Everything below was verified first-hand against the code; gates were re-run on this machine (rustc/cargo 1.99.1) because CI sits at action_required for this author.

Verdict: useful, root-caused, complete for its claimed scope, no smuggled changes — one MAJOR copy-accuracy defect to fix before merge, plus 7 minors.

What held up under fresh verification

  • No smuggling. All 45 hunks across the exact 18-file set attribute to a matrix row, a wave-commit purpose, or a disclosed note; zero whitespace churn; DCO 18/18; no duplicate subjects (the r1 dup is fixed; f835cfebd correctly retitled).
  • Digests byte-verified. The four new SUPERSEDED_SHIPPED_BODY_DIGESTS entries equal the SHA-256 of the base asset files; V4_BEST_PRACTICES_LEGACY_BODY is byte-identical (1772 B) to the base v4 asset; the gen bump + entries together matches the 10→11 (cd8fd0b65) and 11→12 (f1eb10efc) precedent.
  • Retirement repair is live and load-bearing, not dead code. install_system_skills runs at every startup (system.rs:580 via lib.rs:11082); base compared against the then-current body which was the legacy body, so the asset move in this very PR would have stopped retiring installs that still teach read_file/edit_file — the pin fixes that (98c90cd). Multi-generation refresh (11→13) verified working with a chained-digest end-to-end test.
  • Audit-pointer doubt from the previous round is refuted: #112 exists and is MERGED ("fix(tui): align model-facing text with actual behavior"); the gen-history comments in tests.rs check out commit-by-commit.
  • tasks.rs: all 13 rewritten field descriptions verified TRUE against execution paths — model passthrough/required-with-provider, plan read-only mapping, allow_shell omission → durable config.allow_shell() default false (distinct from interactive_allow_shell), trust_mode hardcoded false at from_runtime, auto_approve create-default false, BLOCKED classifier refusal, timeout clamp/kill/timed_out shape, attempt group/index/count/verification/attempt_id/limit clauses including the literal Attempt not found: {id} and newest-first sort_by_key(Reverse(created_at)). Zero bare (action=…) descriptions remain in tasks.rs.
  • shell/read/read_media: every clause TRUE — single merged OS pipe with dup'd write end (true arrival order; stderr empty in snapshot), fresh shell per call, 5 MiB pre-base64 cap + BMP sniffed-then-omitted, from_utf8_lossy decode, READ_MAX_LINES/READ_MAX_BYTES = 2000/50KB, READ_IMAGE_BYTE_BUDGET = 256 KiB shared by auto+low, crop bypassing to MAX_WIRE_IMAGE_BYTES, ladder real, "roughly 350 KB" wire math fair.
  • prompts headers substantively true: message[0] = constitution with the Calm tone literally inside BASE_PROMPT (text.rs:56-60); CALM_PERSONALITY used only by two tests, which do assert it stays folded/absent; approval policy travels as <turn_meta> runtime metadata (engine.rs:4270-4296).
  • fim behavioral change: correct, root-caused, complete. edit (file.rs:1694-1704) and apply_patch (apply_patch.rs:1439) already refuse ambiguity — fim was the outlier; no regression case found (empty anchors now refused consistently with the "exactly once" schema wording; byte-offset slicing char-boundary-safe; perf = two linear scans vs an API round-trip). All three disclosed out-of-scope items verified TRUE, including the exact default_auto_approve line (task_manager.rs:3105).
  • Test quality: 9/9 mutations killed. fim trio RED with the refusal deleted; retire test RED with the legacy pin reverted; forkguard sweep RED on an injected `Bash`; the canonical no_advertised_tool_teaches_a_retired_name guard RED in isolation on the exact historical "Bash with action 'wait'" regression; lsp pin RED. No vacuous tests; the two phantom-name guards are honestly scoped and disjoint.

MAJOR

M-1 — run_tool.rs:110: "Commands run directly as program+args, not through a shell" is false for background=true.
Foreground gates do spawn directly (Command::new(program).args(...), verifier.rs:1156-1160). Background gates do not: render_gate_command shlex-joins program+args into one string (verifier.rs:656-661) and hands it to ShellManager::execute_with_options_env_for_session → CommandSpec::shell (shell.rs:2182) → the platform shell (bash -c / pwsh -Command / cmd /C). The repo's own test comment admits the quoting is lossy in that path ("rendered with POSIX quoting and handed to PowerShell, which cannot parse a quoted absolute path with backslashes", verifier.rs:1496-1502). So for background gates the sentence is false and the shell-routing/quoting caveat is undisclosed — exactly the defect class this PR exists to eliminate, on a load-bearing schema string. Suggested fix (copy-scoped, no behavior change): state that foreground gates run program+args directly while background gates currently render the command through the detected platform shell (so args are shell-quoted; prefer program='bash', args=['-lc', …] there), or align the background path to direct spawning in a separate behavioral PR — either way the unconditional sentence has to go.

Minors

  • m-1 — shell.rs:133/144 (change of this PR): the reworded hint "pass an explicit cwd" is unactionable on the surface that emits it. The visible bash schema (shell.rs:4576-4594) has no cwd property and additionalProperties:false; cwd is published only on the read-only schema (shell.rs:4674) and the hidden compat path (shell.rs:5076). On the main bash tool the actual escape hatch is cd <dir> && … inside command. Same defect class the exec_shell hint fix targeted — suggest "or cd into it in the command" (optionally noting cwd where that parameter exists).
  • m-2 — text.rs:46-49: BASE_PROMPT's doc comment still claims the runtime "composes this core with mode / approval / skills / context-management / compaction / authority-recap layers" — contradicting the corrected assembly story this PR installs 30 lines up (text.rs:16-27) and in prompts.rs:5-8. In-scope residue of 29a1ecb's "finish stale overlay header cleanup" (mode is deliberately absent, approval is turn_meta, compaction is not a system layer). Also pre-existing, outside this diff: prompts.rs:1097-1111 items 5-6 still describe compaction-relay system appends the builder no longer performs.
  • m-3 — tasks.rs:194 (mode): "approvals stay enabled" is false for the reachable plan + auto_approve=true combination. RuntimePolicyProjection::from_request maps auto_approve=Some(true) to ApprovalMode::Bypass with no plan override (runtime_policy.rs:55-57). The read-only headline still holds (shell/writes/MCP-non-readonly stay hard-blocked; sandbox ReadOnly), but the clause as written is wrong on that path. Suggest wording that scopes the claim, e.g. "plan's blocks are not lifted by auto_approve (which otherwise skips approval prompts)".
  • m-4 — run_tool.rs background: "return task_ids immediately" — the literal task_ids array exists only in tool-result metadata (verifier.rs:653), which the engine does not deliver to the model; the model-visible payload carries jobs[].task_id (verifier.rs:219). Wording drift, semantics intact — suggest "returns immediately with per-gate task ids".
  • m-5 — fim.rs:76-84: the tool-level description() omits the uniqueness contract that the schema properties now disclose; sibling edit discloses at both levels (file_tool.rs:163). Convention parity in a copy-completeness PR.
  • m-6 — v4 legacy pin mechanism: a ~1.7 KiB full-body raw-string const in production source diverges from the repo's own precedent (digest in prod + historical bytes in tests: GENERATION_10_DOCUMENTS, tests.rs:652-667; fixtures/mcp-discovery-v11.md, tests.rs:752). Today the pin and the compare target are the same const, so a future transcription drift would keep every test green while real legacy installs silently stop retiring — a digest pin (e.g. crate::hashing::sha256_hex) plus a test fixture would self-check. Behavior verified byte-correct today, so this is a hardening suggestion, not a defect.
  • m-7 — completeness observation: 14 bare "(action=…)"-style descriptions remain in other model-visible tool schemas (automation.rs:149,153,157,174,183; rlm.rs:283,287,293,297; github/schema.rs:26,30,35,56,68(+64 with copy)) — the same copy class this PR eliminates in tasks.rs. Either fold them into this batch or disclose them as out-of-scope; the body's "nine remaining" claim itself is accurate for tasks.rs.

Nits (no action required)

  • f835cfe: SoB display name asto18089 vs author name asto (same email; DCO CI passes, but DCO.md guidance says the trailer should match).
  • 29a1ecb typed fix(prompts) but is doc-comment-only; identical-content sibling 9fbce4d is typed docs(prompts).
  • fim.rs:140-142 comment uses present tense ("are matched at their first occurrence") for behavior the next lines suppress; "would be matched" is exact.
  • fim.rs: the refused() helper is reused by the unique-anchors test where nothing is refused — name overclaims.
  • tasks.rs range restatements ("Defaults to 20; values are clamped into 1-100") duplicate the schema-declared minimum/maximum/default in the same property object — keeping "clamped, not rejected" suffices.
  • tests.rs:86-88 comment "install or keep it" overstates: byte-exact keeps are deleted at next launch (retire now also matches the current body); the current-body retire arm also lost its dedicated test in the de-tautology fix — consider one extra case or a reword.
  • Foreground run_gate misreports a nonexistent cwd as "{program} is not installed or not in PATH" (verifier.rs:1167-1181) while the background path errors accurately — pre-existing, adjacent to m-1's surface.
  • commands[] items lack additionalProperties:false although unknown item keys hard-fail via deny_unknown_fields (verifier.rs:115-122) — the strictness is real but unadvertised.

Gates (re-run fresh on this machine, PR tree @ 9e24145 vs clean f1eb10e)

  • cargo fmt --all -- --check: clean.
  • clippy --workspace --all-targets --all-features --locked: diagnostic sets byte-identical between trees (six pre-existing: 5× too_many_arguments, 1× collapsible_if at turn_loop.rs:5499); a --cap-lints warn completeness pass confirms zero new diagnostics in any crate/target. (The exact command exits 101 on both trees — pre-existing workspace lint deny, unchanged by this branch.)
  • Full suite cargo test -p codewhale-tui --lib --locked (RUST_MIN_STACK raised): 12275 passed / 3 failed / 13 ignored; all 3 failures classified as load-order flakes (pass in isolation; one was cross-process interference from a concurrent lane), zero PR regressions. Every touched-module count claim in the body reproduced exactly (fim 4, skills::system 29, canonical_action+run_tool 10, tasks+file_tool 37, shell trio 3, prompts 116, verifier+fim+forkguard 16 — all 0-failed).
  • 10/10 new and updated tests green by name; 9/9 mutations RED/GREEN as designed.

Recommendation: fix M-1 (copy reword is enough for this PR's scope) and the three one-liner minors m-1/m-2/m-3 if convenient; m-4 through m-7 can ride a follow-up. Everything else — usefulness, completeness, root-causing, elegance, hygiene — held up under fresh adversarial verification.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Wave 2 pushed (9e24145 → 7da8df2, 6 commits) answering this review. Every change was made against the code evidence first, and the round-2 gates were re-run before pushing:

MAJOR

  • 3722e9337 — Run verifier copy scoped per execution path: foreground gates spawn program+args directly; background gates render to one command line through the detected platform shell (quoting caveat disclosed). Also drops the literal task_ids field claim (metadata-only; the model sees per-gate task ids) and adds additionalProperties: false to commands[] items, matching CustomVerifierInput's deny_unknown_fields hard fail.

Minors

  • 6cc287f94 — bash working-dir hint: dropped the unactionable "pass an explicit cwd" clause (the visible bash schema rejects cwd outright; the call is refused pre-spawn so cd cannot help either). Hint test updated together.
  • 47c881fc0 — mode copy now says plan's read-only blocks are not lifted by auto_approve (which otherwise skips approval prompts), instead of the false "approvals stay enabled"; range restatements duplicating schema-declared min/max/default folded to "clamped, not rejected" (schema-undeclared defaults kept).
  • 5ed905783 — fim tool-level description() now carries the uniqueness clause (parity with the edit tool); comment tense fixed; test helper renamed.
  • 92b2180ee — BASE_PROMPT's doc no longer lists runtime mode/approval/compaction layers; volatile-content-last doc rewritten to the real sequence (WorldState fragments + authority/locale trailers); remaining "overlay" wordings marked retired.
  • 7da8df2a5 — v4 legacy pin switched to digest + test fixture per the repo's own pattern: the fixture asserts every run that the historic bytes still hash to the pinned digest, so drift on either side fails the suite (mutation-proven both ways). Restores the current-body retire-arm coverage lost in the de-tautology fix and aligns the two comments.

m-7 (14 bare "(action=…)" fields in automation/rlm/github schemas) is disclosed out-of-scope in the body instead of risked: each needs its execution path traced before truthful copy can be written — flagged as a follow-up batch, alongside the pre-existing foreground run_gate cwd misreport.

Gates re-run post-wave (this machine, rustc/cargo 1.99.1): fmt clean; clippy set byte-identical to base (six pre-existing, none introduced); full suite 12,292 tests — 12,275 passed / 4 failed / 13 ignored with all four failures the known machine-specific load-order flakes that pass in isolation (remote_control ×3, client::operation_tests ×1); touched-module set 199/199 including the canonical guard; new-digest mutations RED in both directions (retire-condition removal, one-byte fixture drift). No fork-guard needle broken.

Housekeeping note: the git endpoint was unreachable at push time, so this wave went up via the Git Data API during a window when api.github.com was still reachable — server-side commit SHAs therefore differ from the locally verified objects while carrying identical trees, messages, authors, dates, and per-topic DCO sign-offs. PR head is 7da8df2a5, still MERGEABLE against pinvou3-clean f1eb10e (rebase remains a no-op).

@h3c-hexin h3c-hexin closed this Nov 4, 2026
@h3c-hexin h3c-hexin reopened this Nov 4, 2026
asto18089 added a commit that referenced this pull request Nov 5, 2026
In-range lock refresh closing all 4 open Dependabot alerts (4 high):

- undici 7.29.1 -> 7.30.1 via wrangler 4.150.0 -> 4.156.1 -> miniflare
  5.20261006.2-beta -> 5.20261101.0-beta in root, web/, and
  telemetry-ingest/: closes GHSA-xjj6-f33q-929c / CVE-2026-109869
  (alerts #139, #142, #145; fetch DoS via unhandled error on excessive
  content-encodings). This closes the follow-up PR #120 left open:
  wrangler 4.156.1 (latest stable) exact-pins miniflare
  5.20261101.0-beta (2026-11-01), which exact-pins undici 7.30.1, so a
  plain `npm update wrangler` per directory lands there. Collateral,
  all npm-canonical: workerd 1.20261006.1 -> 1.20261101.1 (+ its five
  platform binaries), tslib 2.8.1 -> 2.9.1 in root and
  telemetry-ingest, `libc` metadata added to the existing
  @img/sharp-* entries in root and telemetry-ingest, wrangler's
  @cloudflare/workers-types peer range following to ^5.20261101.1,
  web's nested miniflare ws 8.21.0 hoisting to the top level
  (content-identical), and the nested miniflare/node_modules/undici
  copies in web/ and telemetry-ingest/ moving to a single top-level
  7.30.1 entry (root's undici was already top-level). No other package
  versions change.
- @azure/msal-node 6.0.1 -> 7.1.2 in extensions/vscode/: closes
  GHSA-xcx6-jx8p-x3h5 / CVE-2026-109461 (alert #136; auth-code flow
  origin validation; first patched 7.1.0) via the native in-range
  path: @azure/identity 4.14.1 (2026-10-29) requires @azure/msal-node
  ^7.1.0 and satisfies @vscode/vsce's declared ^4.13.2, so
  `npm update @azure/identity` moves identity 4.14.0 -> 4.14.1 and
  msal-node -> 7.1.2 with no manifest change. msal-common follows
  upstream's native shape (top-level 16.14.2 for msal-browser 5.25.1,
  nested 16.15.0 under msal-node; both exact pins). msal-node 7.x
  keeps the confidential-client surface identity uses (jsonwebtoken
  ^9, node >= 20); the path is dev-only (vsce).

Verified:
- npm ci green in root, web/, telemetry-ingest/, extensions/vscode/
- telemetry-ingest: npm run typecheck, npm run check (wrangler 4.156.1
  deploy --dry-run), npm test (171 passed)
- web: npx wrangler --version -> 4.156.1, npm run cf-typegen, npm test
  (408 passed in 47 files)
- extensions/vscode: npm run check (tsc), vsce package
  --no-dependencies (valid 13-file vsix), and an identity -> msal-node
  7 live smoke: ClientSecretCredential.getToken() builds and parses a
  real AAD protocol response through msal-node 7.1.2
- npm audit: root and telemetry-ingest report 0 vulnerabilities; web
  reports 20 moderate advisories (glob via @opennextjs/cloudflare
  1.20.9, minimatch via the eslint toolchain) ingested after this
  branch's initial verification; all devDependencies, none open GitHub
  alerts, left for a separate decision. No HIGH findings remain in any
  of the four refreshed manifests: the base audits carried 1 HIGH for
  undici in root, web/, and telemetry-ingest/, and 2 HIGH for
  msal-node in extensions/vscode (the advisory this bump closes)

Out of scope, unchanged: npm audit reports follow-redirects (high) in
integrations/{feishu,wecom}-bridge and qs + minimatch (moderate) in
extensions/vscode; none of these are open GitHub Dependabot alerts, so
they are left for a separate decision.

Signed-off-by: asto <asto18089@126.com>
The visible `read` tool described itself as text-only, but the contract
reader returns sniffed PNG/JPEG/GIF/WebP files as inline image blocks and
decodes everything else as UTF-8 with replacement characters instead of
refusing invalid bytes (image OCR lives on the hidden legacy read path).
Say so, and keep the 2000-line/50KB default window statement that matches
the contract reader's effective budgets.

Signed-off-by: asto <asto18089@126.com>
The small-contract bash description claimed to "return stdout and stderr",
but the contract path drains both descriptors through one shared pipe and
concatenates them, so the model receives a single merged stream it cannot
attribute per-fd. Each call also spawns a fresh shell: `cd` and exported
variables do not persist between calls. State both facts up front.

Signed-off-by: asto <asto18089@126.com>
v4-best-practices taught `read_file` and `edit_file`, both hidden
compatibility names that never appear in a model-visible catalog or in a
`tool_search` result; hosts that derive allowlists from the wire catalog
reject the call outright. Cite the visible `read`/`edit` tools instead,
extend the existing retired-name guard to this asset (it is not in
BUNDLED_SKILLS, so the bundled-body sweep never covered it), and drop the
stale scope note.

Note: installs whose SKILL.md byte-matches the previously shipped body are
no longer auto-retired by `retire_unchanged_v4_best_practices`; they keep
the stale copy until user-deleted, same trade-off as the prior body edit.

Signed-off-by: asto <asto18089@126.com>
The visible `Run` wrapper reduced the hidden run_verifiers schema to bare
one-line param docs, dropping the load-bearing clauses: custom gates run
directly as program+args, not through a shell (reach for `bash -lc` only
intentionally), and background gates return task_ids immediately with
completion tracked in task/status state rather than pollable streams.
Carry the hidden schema's wording into the visible surface, including
per-field descriptions.

Signed-off-by: asto <asto18089@126.com>
fim_edit matched both anchors at their first occurrence with no uniqueness
check, so a repeated anchor silently generated the middle at the wrong
site. Count occurrences and refuse the edit when the prefix anchor repeats
anywhere or the suffix anchor repeats after the prefix, naming the match
count and asking for a longer anchor; disclose the exactly-once contract
in the parameter schema. Covered by three new tests.

Signed-off-by: asto <asto18089@126.com>
The description and `detail` schema advertised the 5 MiB payload ceiling
but not the 256 KiB delivery budget a default read ('auto'/'low') actually
applies, while 'high'/'original' and crop reads keep the full budget.
State both numbers where the model chooses a detail level.

Signed-off-by: asto <asto18089@126.com>
tasks create advertised `allow_shell`, `trust_mode`, and `auto_approve`
with bare "(action=create)" placeholders. Document what each authority
flag grants (shell permission with session-default inheritance, relaxed
workspace-boundary path validation, auto-approved tool calls) and that a
gate_run command the safety classifier rates dangerous is refused with
BLOCKED before anything runs unless the session itself auto-approves.

Signed-off-by: asto <asto18089@126.com>
The line-based frontmatter parser reads flat key/value pairs and no crate
consumes `short-description`; the nested `metadata:` block only parsed as
a no-op "metadata" key plus a stray top-level entry. Remove the four dead
blocks; names, descriptions, and aliases-for are untouched.

Signed-off-by: asto <asto18089@126.com>
The module header listed the personality overlay as assembly step 2, but
the runtime folds personality tone into the constitutional preamble and
never composes CALM_PERSONALITY (tests pin the Calm overlay out of the
default prompt). Describe the real assembly order and mark the constant
as retired test-only text.

Signed-off-by: asto <asto18089@126.com>
definition and references fail with "requires line (1-based)" when the
line parameter is missing, but the schema described it as a plain
"1-based line." with no requirement. Name the operations that need it
and pin the string in the schema test.

Signed-off-by: asto18089 <asto18089@126.com>
Nine task-tool properties still carried bare "(action=...)" descriptions:
model, mode, timeout_ms, the attempt bookkeeping fields, verification,
attempt_id, and limit. Describe each from its actual execution path
(defaults, clamping, failure shapes).

Also correct the fresh allow_shell copy: omitting the flag resolves
through the durable-task profile snapshot (config allow_shell(), default
false), not the interactive session's effective setting.

Signed-off-by: asto18089 <asto18089@126.com>
retire_unchanged_v4_best_practices compared installed copies against the
current asset only, so the PR edit that fixed read_file/edit_file in the
shipped body stopped retiring exactly the installs that need it: those
still teach retired tool names. Pin the previously shipped body as a
legacy const and retire byte-exact matches of either body; user edits
stay.

The retirement test wrote the same const the retire fn compares against,
so its assertion was a tautology and no longer simulated any real
install. Write the legacy bytes instead and pin the two constants
apart; an appended-line case proves only byte-exact bodies retire.

Signed-off-by: asto18089 <asto18089@126.com>
…tion cleanup

The four bundled SKILL.md bodies stripped of dead short-description
frontmatter changed shipped bytes without the non-destructive upgrade
half: no superseded digest entries and no generation bump. Per the
codewhale-hq#4691 convention (bump plus entries together, as 10->11 and 11->12
did), bump to generation 13 so installed gen-12 copies refresh at next
launch, and record the four previous-body digests so those copies count
as unmodified shipped text instead of user edits.

Signed-off-by: asto18089 <asto18089@126.com>
The previous header fix corrected the personality line but kept
renumbering "Approval-policy overlays" as a composed layer; those
overlays were removed as dead code and approval policy travels as
request-time runtime metadata. Drop the layer and say where approval
policy actually lives; align the prompts.rs module header, which still
named a personality overlay step.

Signed-off-by: asto18089 <asto18089@126.com>
The read description claimed images unconditionally, but the inline
path caps at 5 MiB (larger images come back as an omission note) and
BMP is sniffed into the image branch yet rejected by the validity
check, so its bytes never reach the model as text or image. Name both
cutoffs; the sibling read_media description already discloses the same
5 MiB limit.

Signed-off-by: asto18089 <asto18089@126.com>
validate_shell_working_dir told the model to pass an explicit
working_dir/cwd "to exec_shell" — a name retired in v0.9.3 that no
catalog or tool_search result can return. The hint also offered
working_dir, an alias only the hidden compatibility tool accepts. Ask
for an explicit cwd, which the surfaces that reach this path can set.

Signed-off-by: asto18089 <asto18089@126.com>
asto18089 and others added 15 commits November 5, 2026 10:26
'deliver about 256KB' read as a typical delivered size; small images pass
untouched at far less and the ladder delivers at most the budget. Use the
same budget framing the low clause and docs already use.

Signed-off-by: asto <asto18089@126.com>
…cisely

The volatile-content-last enumeration omitted two WorldState fragments
the builder really adds (the interactive continual-harness block and the
imported project-instructions fragment), and the module header's 'single
text module' overstated where every compile-time constant lives.

Signed-off-by: asto <asto18089@126.com>
"Out-of-range values are clamped, not rejected" was false for negative
integers: they are rejected as a type mismatch by the u64 parser and
never reach the clamp. State the boundary per field instead (clamped
into the declared range; below-1 clamp for the attempt fields), and pin
the corrected sentences plus the reviewed authority claims (mode blocks
not lifted by auto_approve, durable-task allow-shell fallback, trust
mode full-access retry, BLOCKED gate refusal) that had no schema pin.

Signed-off-by: asto <asto18089@126.com>
The receipt printed the pre-base64 encoded size as "(wire payload)", but the actual wire payload is the base64 form (~4/3 larger) — the same doc the budget bullet cites says so. Name it honestly.

Signed-off-by: asto <asto18089@126.com>
Three comments survived the assembly-layer cleanup with stale claims:
the section header still said every bundled layer lives in text.rs
(the authority recap and locale bookends are defined here), the handoff
doc still said the relay artifact is prepended to the system prompt
(it is routed as the late WorldState token-budget fragment), and the
environment-block doc still claimed placement before instructions and
skills (it leads the workspace WorldState fragment, after the
constitution prefix). Mirror the corrected module header in text.rs.

Signed-off-by: asto <asto18089@126.com>
Cover the emergent counting semantics the refusal trio left unpinned:
an empty prefix is refused with the char-boundary count (characters +
1, not bytes + 1 — the multi-byte fixture discriminates a byte-offset
refactor), suffix occurrences before the prefix are invisible to the
uniqueness check, and a suffix that only occurs before the prefix
stays SuffixNotFound.

Signed-off-by: asto <asto18089@126.com>
The visible background copy promises a task id per gate, null when a
gate is skipped or fails to start; only the running arm had coverage.
Drive both null arms directly through start_background_gates: a gate
without a program reports skipped with its reason, and a gate whose
working directory does not exist fails shell-manager validation and
reports failed_to_start with the accurate error.

Signed-off-by: asto <asto18089@126.com>
The Run schema's per-path shell scoping and background task-id copy,
the bash tool's merged-stream/fresh-shell claims, and the read tool's
image/decoding disclosures had no test anywhere, so a full revert of
any of them was invisible to CI. Pin each corrected sentence (and the
absences: no task_ids array in the visible background copy, no
separate-stream promise, no text-only read claim) against the schema
and description strings.

Signed-off-by: asto <asto18089@126.com>
The shape test only rejects a digest equal to the current body, so a
single wrong hex digit in one of the four generation-13 entries would
silently leave real generation-12 installs unrefreshed. Embed the four
previous bodies as fixtures (the same pattern as the v4 legacy pin and
the generation-10 documents body) and hash each against its registered
entry on every run; the behavioral supersede check rides along.

Signed-off-by: asto <asto18089@126.com>
The model-visible harness tool's description and its overview result's
runtime block told the model to keep large source material in rlm. rlm
is compatibility-only (model_visible() == false, kept out of the catalog
and tool_search, and its own description says not to teach it to new
model turns) — the same phantom-name class this branch fixed in the
v4-best-practices skill and that the PHANTOM_NAMES sweep guards. On
hosts whose allowlists derive from the wire catalog, the recommended
call is rejected outright.

The copy now names the visible bounded-reread surfaces instead: read
with offset/limit and retrieve_tool_result.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
std matches() counts non-overlapping occurrences, so a self-similar
anchor ("aa" inside "aaa") passes the uniqueness check with count 1
and the edit lands at the first occurrence. Stated bare, "must appear
exactly once" reads as overlapping occurrences to a model counting with
overlapping intuition — the sibling edit tool already discloses its
"unique, non-overlapping region" contract, so fim now says the same at
both levels (tool description and both anchor schema descriptions) and
a copy test pins all three sentences.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
An empty anchor is not special-cased: the refusal emerges from the
general matches-more-than-once rule. The two corners therefore pass
with count 1 and keep the base behavior — an empty prefix on an empty
file (single char boundary) and an empty suffix after a prefix ending
at EOF (empty suffix region). Both now pin that they reach the API
boundary instead of a refusal; the round-4 note in the PR body had
claimed this pinning existed, but no such tests landed with that wave.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The round-4 fix that relabeled the receipt's pre-base64 size from
"(wire payload)" to "(encoded payload)" carried no test, so a silent
revert would be invisible to CI. The delivery-note test now asserts the
encoded-payload label is present and the wire-payload wording is gone.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
"Passing true or false is always honored" was an absolute: under
mode=plan the value is stored but shell stays disabled by the mode's
own hard block (the mode property already discloses exactly that
precedence). The allow_shell copy now says "honored subject to the
mode", naming the plan carve-out, and the authority-copy pin test
asserts both new sentences.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The module header still ended on "Keeping every layer's text in one
module makes prompt tuning a single-file operation." — contradicting
the corrected constant-split header directly above it (authority recap
and locale bookends live in this module) and surviving two header
cleanup waves. The handoff test whose name still said "prepended" is
renamed to what it actually pins (inclusion); the prepending doc claim
it echoed was corrected in the round-4 wave.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
@asto18089
asto18089 force-pushed the fix/model-facing-copy-residue branch from 74e8187 to 71b61ea Compare November 5, 2026 07:45
asto18089 and others added 9 commits November 5, 2026 15:54
The contract reader now refuses NUL-bearing payloads (any NUL in the
first 8 KiB) and UTF-16 BOMs as binary before the lossy decode
(contract_read_payload_is_binary, added to file.rs by the
subagent-isolation work while this PR was in review), so the read
description's 'bytes that are not valid UTF-8 become replacement
characters instead of failing the read' was an absolute the engine no
longer honors: stray high bytes and NUL-free heads still decode lossily,
the refused classes do not. Scope the clause with the refusal exception,
pin the disclosure in the description pin (and forbid the unscoped
sentence end from coming back), and correct the refusal message's
'read returns text only' absolute - read returns inline images for
sniffed formats - to 'read returns text or inline images, never raw
binary', pinned in the behavioral refusal test.

Signed-off-by: asto <asto18089@126.com>
The wave-5 harness fix (stop teaching the hidden rlm compatibility
surface; name the visible bounded-reread surfaces instead) landed with
copy only - no test guarded it, so re-adding both original rlm mentions
passes every guard and suite (mutation-proven in review). Pin the
removal in both model-visible strings: the description and the overview
result's runtime.context must carry no rlm and must keep naming read
with offset/limit and retrieve_tool_result.

Signed-off-by: asto <asto18089@126.com>
Two header residuals the wave cleanup missed: the consolidation sentence
still claimed in the present tense that the whole prompt contract reads
top-to-bottom in this one module, contradicting the corrected header
directly above it - the authority recap and the locale bookends never
lived here, so the claim is now scoped to the constants that moved. And
the edit-loop command ran 'cargo test -p codewhale-tui --bin
codewhale-tui prompts', which matches zero tests (the prompts tests live
in the lib target); it now names --lib.

Signed-off-by: asto <asto18089@126.com>
Base #131 changed the approval pump to auto_approve &&
!approval_force_prompt, so on a host that arms ask-on-guardrail
guardrails, guardrail-class calls force a real human approval even for
an auto_approve=true task. The "no approval prompts" absolute (and the
mode row skip parenthetical) now carry that exception, the mode row
discloses that trust_mode's effects still apply in plan (the retry path
has no mode re-check), and the pins assert the exception clauses and
forbid the unscoped absolute from returning.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The tasks description pointed at task_shell_start unconditionally,
but the registry registers the runtime task shell tools only under
ShellPolicy::Full (allow_shell on, non-plan mode) — on the default
durable-task surface the named tool is absent from the catalog. The
task_id property grouped read with cancel as requiring an id, while
read resolves through the same active-task default as pr_attempt_*.
Both sentences are scoped and pinned.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The item-5b budget sentences on the description and the detail
schema were the one audited copy surface with no revert guard; a
mutation probe flipping the figure passed the whole suite. Both
surfaces are now pinned.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The schema copy discloses that anchor occurrences are counted
without overlap, but no test exercised a self-overlapping anchor, so
an overlapping-counting refactor would have kept every test green
while making the live copy false. "aa" in "aaa" has exactly one
non-overlapping site and reaches the API boundary; an overlapping
counter refuses.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
The requested-directory arm (the model-reachable one) only forbade
the literal "resume/fork" token on the saved-workspace path, so a
paraphrased revival of the false advice there passed CI; the pin now
forbids resume and fork wording on that arm outright. The
saved-workspace not-a-directory branch (shell.rs Restore-it advice)
had no test at all and gets one.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
BASE_PROMPT's doc summary listed sections the constant does not have
(output format, toolbox reference, "When NOT to use", sub-agent
sentinel protocol live elsewhere), and the AUTHORITY_RECAP doc claimed
it is appended at the end of the system prompt when the locale closer
follows it for non-English locales — it is a per-request trailer after
the WorldState fragments.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

round-8 从零复审终局:APPROVE 级(零 P0/P1)

注:本 PR 由本账号推送,GitHub 禁止作者 self-approve,故正式 review 无法提交;此为本轮评审最终结论,以评论形式发出(本仓 reviewDecision 不作门禁,以 CI required gate 为准)。

从零 19-lane 全新复审,锚定 head eab45d4aa(8 个逐面真相 lane:read / read_media / bash / fim / Run / tasks / skills / prompts;8 个横切 lane:提交归属走私审计、正文真值、变异探针、本地测试套件、fork-guard、base 交互、披露真值、工程惯例;3 个兜底 lane:性能、CI 取证、全 diff 新鲜眼)。

Rebase:merge-base = base tip 11cba403d,分支零漂移,无需操作。

关键复核全部通过:

  • 逐面真值:head 上未发现任何虚假 model-facing 文案;wave-7 armed-guardrail 修复独立复核闭合(runtime_threads.rs:9498 旁路 + 双 pin 禁回潜)。
  • 变异探针 8/8 RED→恢复 GREEN:wave-6/7 全部 pin 真实有效(含 armed 异常正/负 pin、预算句、fim 非重叠行为 pin、shell 建议两 arm)。
  • SHA-256 digest 5/5 两条 lane 独立重算全对;fork-guard 216/216 CodeWhale 指纹全 present(唯一 miss = base fix(subagent): 固定任务身份与完整输出恢复 #124 继承搬迁,非本分支回归);forkguard 标识符 head/base 集合逐字节相同(301==301)。
  • 本地验证:fmt 清;touched-module 过滤 596 passed / 0 failed;forkguard 285/0(单线程);no_advertised_tool_teaches_a_retired_name 绿;clippy base/head 字节相同(8 个既有诊断,exit 101 双树一致);runtime_api 栈溢出 abort 在 base 树同样复现,正文"机器特定"披露属实。
  • 走私 CLEAN:53 提交全归属、一线性史一主题、DCO 全过;正文 41 个 SHA 与 16 处行号引用全部精确。
  • 性能 NEUTRAL:catalog +5.1KB 一次性注册构建、fim 线性扫描先于网络调用、digest 启动微秒级。
  • CI:head 11 绿零红;sync/CodeQL 两个 skipping 结构性正常(与 fix(tui): drop stale mode copy from locale packs #134 等已合并 PR 同型)。

P2 非阻塞台账(1)

  1. crates/tui/src/skills/system.rs:174-177 is_legacy_v4_best_practices_body 与同文件 :102-104 is_superseded_shipped_body 逐字节重复 sha256-hex 三行块。建议提 3 行 body_sha256_hex helper 去重,后续轮次即可。

P3 非阻塞台账(要点)

  1. 正文 Verification "27 registered needles touching files this branch modifies" — 按 wave-6 后 29 文件 delta 实为 30(3 针落在 file.rs/file/tests.rs),全部 present,仅计数建议更正。
  2. Run 可见 background 描述与 poll_with 元数据无条件指向 task_shell_wait,但该工具仅 ShellPolicy::Full 下注册(registry.rs:1266-1273)— 与 round-7 tasks task_shell_start 指针修复同族,较轻(使用提示非调用推荐),可留后续。
  3. tasks auto_approve 行只披露 armed-prompt 面:unarmed 主机上 guardrail 类调用在 auto_approve=true 下是硬 deny 非 prompt(turn_loop.rs:4009-4013 等)— 现文案字面为真("no approval prompts" 对 prompts 成立),属 understatement,可后续补一句。
  4. fim matches().count() 每次全缓冲扫描,take(2).count() 可早退(先于网络调用,纯理论优化)。
  5. read_media 单位混写:description "~256KB" vs docs/READ_MEDIA.md "256 KiB" vs "roughly 350 KB",建议统一口径。
  6. 同族残留候选(均 pre-existing / 用户面,非本 PR 引入):tools/handle.rs:250 handle_read 描述提及 "RLM sessions";tui/ui/session_state.rs:179 用户面 toast "Use exec_shell_cancel"。可与正文已披露的用户面 follow-up 批次合并处理。
  7. file_tool.rs:63 decode 句 ~60 词嵌套从句为全 PR 最难解析句,后续轮次可拆句。

合并后动作(parent 仓)

APPROVE 级 — 建议进 merge queue。

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

已审查固定 head eab45d4aa8fe8ed4a574e52fb5af2310e7adef33,base 11cba403dc6bd6ca5c4dca55042785b03449ac8e。完整 29 文件差异的规范与需求两个轴均未发现实质问题,批准本 PR。

已核对模型文案对应的实际读取、媒体预算、shell、Run、tasks 与提示词组装路径;FIM 的非重叠计数/后缀搜索区间与披露一致,歧义拒绝发生在 provider 调用和写入前。Skill generation 13 的四份 gen12 fixture 及 v4 历史 fixture 均逐字节匹配 base 资产,五个 SHA-256 均与迁移登记相符,用户修改保护路径保持。

实际验证:fetch、完整源码/测试内容与 consumer 审查、git diff --check、五份历史资产的字节/摘要计算和 JSON catalog 解析,并只读核对该 head 远程检查(完成的检查成功,CodeQL neutral、sync skipped)。未在本机执行 Cargo/Rust、FIM 实际生成/写入、启动迁移或原生图像/provider 测试;作者的大规模测试/变异计数不是本轮执行结果。父仓 fork 登记、指纹调整和 gitlink 集成仍由对应父仓变更处理,未声称已验收该集成。发送前已核对最新 head/base 及讨论未变化。

@zhuowp zhuowp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approve the source change at eab45d4aa8fe8ed4a574e52fb5af2310e7adef33 against actual target 11cba403dc6bd6ca5c4dca55042785b03449ac8e.

The cumulative 29-file/108-hunk scope and 35 unique affected registered test bodies are covered. I checked the actual tool registration, prompt/skill loading, migration preservation, FIM refusal-before-provider/write, media budgets, Run execution paths and task-policy consumers. No blocking defect was confirmed.

Two nonblocking P3 notes:

  • Pin the five new historical Markdown fixtures to LF. In a normal core.autocrlf=true checkout these unpinned include_str! inputs retain CRLF, while the two historical digest self-checks require LF hashes. This is a test-portability defect, not an observed Windows CI failure or proof of a new production migration regression: the actual target's shipped assets already have LF attributes.
  • Scope the new tasks.timeout_ms promise to the platform guarantees. The Unix path kills a process group; the non-Unix fallback kills the direct child and retains the actual exit-status code. The timeout/status behavior itself is unchanged here.

Exact-tree Linux CI run 42173780690 / check 127747440420 contains ordinary PASS output for all 35 affected names. Its aggregate is 15026 passed (1 slow, 2 leaky), 15 skipped; the two unrelated LEAK warnings remain a resource-validation boundary. Current checks contain 12 success, one CodeQL neutral and one sync skip. Author-reported earlier runs/mutations are not treated as this review's execution.

No new local runner, compile, probe, model/provider or browser was used. Windows/macOS execution, live FIM generation and installed-user upgrades were not performed. This source approval does not authorize merge or release; final live-version and repository gates still apply.

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.

4 participants