Skip to content

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

Open
drewstone wants to merge 1 commit into
mainfrom
feat/policy-edit-author-projection
Open

fix(campaign): preserve proposer state and private profile fields#357
drewstone wants to merge 1 commit into
mainfrom
feat/policy-edit-author-projection

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

Composite proposal labels hid member state, causing stateful methods to repeat candidates. Policy-edit authoring also needed a safe way to omit private profile fields from model context.

Solution

  • restore each member's own labels when replaying composite history
  • add current-surface projection with editable-path transparency checks
  • apply edits to the complete original profile
  • reject unsafe projections before model dispatch

Verification

  • 2,937 tests passed, 2 skipped
  • typecheck, lint, build, OpenAPI, and package export checks passed
  • independent review finding for hidden editable subtrees is covered by a regression test

@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 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

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