Skip to content

fix(campaign): preserve proposer state and private profile fields - #357

Merged
drewstone merged 5 commits into
mainfrom
feat/policy-edit-author-projection
Jul 22, 2026
Merged

fix(campaign): preserve proposer state and private profile fields#357
drewstone merged 5 commits into
mainfrom
feat/policy-edit-author-projection

Conversation

@drewstone

@drewstone drewstone commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

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

  • Restore each member's original labels before both propose() and decide().
  • Reject empty, duplicate, or colon-bearing member kinds because their history ownership is ambiguous.
  • Add redactCurrentSurfaceForModel(surface); it receives a clone, must preserve every editable path, and only changes the current surface sent to the model.
  • Validate redacted JSON before any model call and return specific errors for invalid keys or hidden editable paths.
  • Apply accepted edits to the complete original profile, so redacted credentials and runtime config remain intact.
  • Export AgentProfileJsonObject so the callback's object-only contract is explicit in TypeScript.

Verification

  • Focused tests: 43 passed.
  • Full tests: 290 files passed; 3,307 tests passed, 2 skipped, 3,309 total.
  • 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.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

value-audit · 20260713T073618Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 41e62eeb

Review health 100/100 · Reviewer score 83/100 · Confidence 75/100 · 4 findings (4 low)

glm: Correctness 83 · Security 83 · Testing 83 · Architecture 83

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision.

🟡 LOW decide path with prefixed labels has no direct test coverage — src/campaign/proposers/composite.test.ts

The decide method at composite.ts:137-144 now also calls historyForMember, but the only decide test (composite.test.ts:163-177) passes empty history { history: [] }, so the label-stripping in decide is never exercised with non-empty prefixed history. The logic is identical to propose (same historyForMember function), so correctness is likely fine, but a stateful member that uses decide with label-based convergence logic (e.g., FAPO's policyState) would benefit from an explicit test with non-empty history containing prefixed labels.

🟡 LOW historyForMember prefix collision when member kinds share a colon-prefix relationship — src/campaign/proposers/composite.ts

The prefix-stripping uses candidate.label?.startsWith(prefix) where prefix = ${memberKind}:. If two members have kinds where one is a colon-prefix of the other (e.g., kind 'a' and kind 'a:b'), a candidate from member 'a:b' labeled 'a:b:foo' would match the prefix 'a:' for member 'a', incorrectly stripping it to 'b:foo'. Standard proposer kinds (gepa, skill-opt, parameter-sweep, etc.) don't contain colons, and the prefix-adding logic at line 115 already has this same ambiguity, so this is pre-existing not new. But there's no validation on proposer kinds to enforce this. Fix: validate at composite construction time that no member kind contains ':', or use

🟡 LOW Error message for whitespace-key rejection is misleading but test correctly matches implementation — tests/campaign/llm-policy-edit-proposer.test.ts

The test projects { ...surface, ' mcp': {} } (key with leading space). The JsonObjectKeySchema refine rejects it, but projectAuthorSurface throws the generic 'projectAuthorSurface must return a JSON object' — the return IS an object, just with an invalid key. The test correctly matches the implementation's error message, so this is not a test bug. However, the error message gives no signal to the caller about WHY the object was rejected (whitespace key vs non-object return). Consider a more specific error for schema-validation failure vs type-check failure. Low impact since this is a developer-facing configuration error.

🟡 LOW Task-identifier-hiding test passes for the wrong reason — post-projection assertSurfaceIsTaskAgnostic is untested — tests/campaign/llm-policy-edit-proposer.test.ts

The test sets currentSurface to '{"prompt":{"systemPrompt":"Handle private-task"}}' with baselineOutcome scenarioId 'private-task' and scenarioIdTransform () => 'task-1'. The proposer's first assertSurfaceIsTaskAgnostic (source line 503-506) serializes the wrapper { currentSurface, allowedJsonPaths, objectives, targetSurface } which contains 'private-task', and sanitize() replaces it with 'task-1', so sanitize(serialized) !== serialized throws 'raw scenario identifier' BEFORE projectAuthorSurface is called (source [line 507](https://github.com/tangle-network/agent-eval/blob/41e62eeb99aa36cf3c4efe8ded69c19db7068c24/tests/campaign/llm-policy-


tangletools · 2026-07-13T07:50:17Z · trace

tangletools
tangletools previously approved these changes Jul 13, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 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 a vs ab cannot collide because labels are a:... vs ab:...). 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.

value-audit · 20260722T224949Z

@drewstone
drewstone merged commit da017f8 into main Jul 22, 2026
1 check passed
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.

2 participants