fix(campaign): preserve proposer state and private profile fields - #357
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 41e62eeb
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-13T07:26:23Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 448.0s (2 bridge agents) |
| Total | 448.0s |
💰 Value — sound-with-nits
Fixes a real stateful-proposer-in-composite bug and adds a well-guarded opt-in surface projection for private fields; both are correct and in-grain, with only minor helper duplication.
- What it does: Two independent fixes. (1) composite.ts now strips each member's provenance prefix from history labels before passing history to that member, so stateful proposers like parameterSweepProposer recognize their own prior candidates and skip them instead of re-proposing every generation (historyForMember at composite.ts:161-171, applied in both propose:94-98 and decide:140-144). (2) llm-policy-edit.ts
- Goals it achieves: (1) Stop stateful members inside a composite from repeating already-tried candidates — the composite's label-prefix scheme (memberKind:label) was hiding each member's own labels from its own dedup logic (parameterSweepProposer.triedLabels at fapo.ts:682 compares candidate.label, which after prefixing no longer matches). (2) Let callers keep credentials/private config out of LLM author context with
- Assessment: Both fixes are correct, well-scoped, and match the codebase's established patterns. The composite fix works within the existing prefix-based provenance scheme (documented at composite.ts:16-19 as intentional), stripping exactly one prefix level so nested composites also work. The projection feature is the right abstraction level — it is caller-controlled and structural (can drop entire subtrees),
- Better / existing approach: Searched for existing JSON-path and deep-equal utilities: readJsonAtPath + splitPath exist as private functions in policy-edit.ts:613,865 (functionally identical to the new readJsonPath at llm-policy-edit.ts:1293), and canonicalJson exists as a private comparator in fapo.ts:746 (could substitute for jsonValuesEqual). Neither is exported, so the new helpers couldn't import them. Also checked trace/
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
Fixes a real stateful-member label collision bug in compositeProposer and adds a safe, opt-in credential-stripping projection to llmPolicyEditProposer — both correctly wired into the substrate's public surface.
- Integration: Both proposers are exported from src/campaign/index.ts (lines 241, 274), documented in docs/improvement-glossary.md and CHANGELOG.md, and verified by scripts/verify-package-exports.mjs. They are substrate primitives consumed by downstream packages (agent-runtime, products) — standard for this repo's layering (CLAUDE.md: 'agent-eval has zero upward dependencies'). No internal non-test callers exist
- Fit with existing patterns: Both changes follow established codebase patterns. compositeProposer already does label-prefix provenance (composite.ts:115) and fapoProposer does its own label wrapping (fapo.ts:388) — historyForMember is the natural un-prefixing complement. projectAuthorSurface complements the existing scenarioIdTransform pseudonymizer (llm-policy-edit.ts:420) which only handles scenario IDs, not arbitrary priva
- Real-world viability: The composite fix handles null labels via optional chaining (composite.ts:166) and non-matching prefixes via pass-through. The projection has comprehensive edge-case coverage: non-object rejection (test ~line 318), hidden editable-path rejection (~line 343), mutation prevention (~line 375), whitespace-key rejection (~line 401), and task-identifier leak prevention (~line 413). structuredClone (line
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 readJsonPath and jsonValuesEqual duplicate private helpers in sibling modules [duplication] ``
readJsonPath (llm-policy-edit.ts:1293) reimplements the exact dot-path traversal of readJsonAtPath + splitPath in policy-edit.ts:613,865. jsonValuesEqual (llm-policy-edit.ts:1305) reimplements structural deep-equal that canonicalJson in fapo.ts:746 could provide via canonical-string comparison. All three are private. The codebase now has three independent JSON-path micro-utilities. Not ship-blocking — consider extracting a shared src/json-path.ts (readPath, deepEqual) and exporting it from the s
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 4 non-blocking findings — 41e62eeb
Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-13T07:50:17Z · immutable trace
…or-projection # Conflicts: # CHANGELOG.md
…or-projection # Conflicts: # CHANGELOG.md
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 9514ae6a
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-22T22:47:55Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 0 (none) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 91.5s (2 bridge agents) |
| Total | 91.5s |
💰 Value — sound
Two coherent fixes — composite proposer restores per-member history ownership, and LLM policy-edit gains a validated redaction callback — both correct and in-grain.
- What it does: Two changes. (1) compositeProposer now strips its
kind:prefix from candidate labels before passing history back to each member (in both propose() and decide()), so a stateful member sees its own prior labels verbatim; it also rejects empty/duplicate/colon-bearing member kinds and non-finite weights. (2) llmPolicyEditProposer gains an optional redactCurrentSurfaceForModel(surface) callback: the - Goals it achieves: (a) Stop stateful proposers (e.g. parameterSweepProposer) from repeating work when run inside a composite, because they could not recognize their own prefixed labels in history. (b) Stop leaking credentials (e.g. MCP Authorization headers) into the model prompt without breaking the executable profile — the model sees a redacted view, edits land on the complete original.
- Assessment: Both fixes are correct and sit cleanly in the codebase's grain. The prefix-strip in historyForMember is the honest inverse of the prefix added at composite.ts:108; the colon-as-separator rule plus duplicate-kind guard make per-member history ownership unambiguous (kind
avsabcannot collide because labels area:...vsab:...). The redaction callback mirrors the existing scenarioIdTransfor - Better / existing approach: none — this is the right approach. Searched for an existing deep-equal / JSON-path / structured-redaction helper to reuse (grep for readJsonPath|deepEqual|jsonEqual|redact|sanitize across src/): the only redaction infrastructure is trace-level (src/trace/redact.ts, defaultProviderRedactor in raw-provider-sink.ts) which operates on serialized string payloads and header/body field keys, not on a str
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content
🎯 Usefulness — sound
Composite state-preservation fix solves a real infinite-reproposal bug on a production path (parameterSweepProposer), and the redaction callback is a well-validated opt-in seam that follows the established scenarioIdTransform pattern.
- Integration: Both changed proposers are exported via src/campaign/index.ts:255,288 and reach real callers. The composite fix is exercised by parameterSweepProposer (fapo.ts:652) which is wired into presets/compare-proposers.ts:432 — a production FAPO preset. historyForMember (composite.ts:154) strips the member-kind prefix before handing history back, without which triedLabels (fapo.ts:682) would never recogni
- Fit with existing patterns: Fits the codebase grain. redactCurrentSurfaceForModel mirrors the existing scenarioIdTransform callback (llm-policy-edit.ts:430) — same shape: opt-in caller transform on model-facing data, validated before dispatch. It does NOT compete with trace/redact.ts (redactValue/redactString), which is regex-based PII scrubbing of persisted trace payloads at a different layer. AgentProfileJsonObject is a co
- Real-world viability: Holds up under stress. Redaction clones before the callback (structuredClone), validates return shape via JsonValueSchema, enforces every allowedJsonPath unchanged via deep equality (jsonValuesEqual), and re-runs assertSurfaceIsTaskAgnostic on the redacted surface (llm-policy-edit.ts:523) to catch leaked scenario IDs. Tests cover non-object return, hidden editable subtree, mutating callback, white
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
No concerns — sound change, no better or existing approach found. ✅
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Problem
Composite candidate labels hid each member's prior state, so stateful methods could repeat work. The model-based policy editor also needed a safe, explicit way to remove credentials and unrelated fields from its current-surface input without changing the executable profile.
Changes
propose()anddecide().redactCurrentSurfaceForModel(surface); it receives a clone, must preserve every editable path, and only changes the current surface sent to the model.AgentProfileJsonObjectso the callback's object-only contract is explicit in TypeScript.Verification
pnpm lint: 528 files, 0 errors, 0 warnings.pnpm typecheck: passed.pnpm typecheck:examples: passed.pnpm build: passed.pnpm verify:package: passed against the packed archive.git merge-tree --write-tree origin/main HEAD: clean.