Show Context Tree influence in managed and BYO responses - #2204
Conversation
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: the new behavior gate does not follow the shipped BYO activation contract and does not observe every managed surface on which the note is forbidden, so it can both reject compliant runs and accept duplicate/progress output.
Risk level: B-low
- Path baseline: C — shared schema comments, Web preview, skill payload, and skill-eval files.
- Semantic lift: C → B-low — this versions and rolls out a cross-consumer skill-output contract.
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: managed First Tree users and BYO coding-agent users currently see different evidence that Context Tree decisions influenced an answer.
- Approach: replace new metadata receipts with one compact, localized, exact-version Markdown note in the authored result while retaining legacy receipt readers for stored history and older agents.
- Impacted modules:
skills/first-tree-read,packages/skill-evals, shared legacy receipt schema comments, and the Web preview fixture.
Review findings
❌ 1. The BYO evaluator still recognizes and requires the legacy tree read sequence, while the skill's actual SCOPE-routed workflow calls context route followed by context snapshot. isReadActivationArgv only matches tree read, READ_HELP_ARGV still requires tree read --help, and the sequence never asserts the route call. A compliant model following the skill therefore records zero activations and fails; the unit test passes only by exercising the obsolete sequence. Please grade and test the exact context route → context snapshot → hierarchy help → no-pull selectors contract, including the snapshot result's exact commit. [packages/skill-evals/src/suites/first-tree-read/metrics.ts:346]
❌ 2. The managed exactly-once/final-only check drops the very progress surfaces the skill forbids. isChatAuthoringArgv collects only chat send/chat ask, and once one exists visibleOutputTexts discards all model output; the shim also records no body for chat update --description. A run can therefore place the note in the visible Current state/progress output and repeat it in the final send while impactNoteCount remains 1. Please collect/check status-update and managed progress text for forbidden notes while still requiring the one allowed note at the end of the final send/ask. [packages/skill-evals/src/suites/first-tree-read/metrics.ts:364]
Action taken
- Submitted request changes.
yuezengwu
left a comment
There was a problem hiding this comment.
Review outcome: request changes
The implementation goal is clear: replace new contextDecision metadata production with one portable, localized, exact-version Markdown note in both managed First Tree Chat and BYO coding-agent results, while retaining legacy receipt readers for stored history. The core changes are the first-tree-read 0.6.0 contract plus behavior-oriented eval parsing/cases; the shared schema and Web edits are compatibility comments only. There is no core data-structure or database change in this diff.
I independently confirmed the two blockers already reported by @baixiaohang:
packages/skill-evals/src/suites/first-tree-read/metrics.tsstill grades the legacytree read --help/tree readactivation sequence. The shipped skill requirescontext routefollowed bycontext snapshot, but neither call participates inreadActivationSucceededorbyoReadSequenceOk. A compliant BYO run therefore fails the gate, while the unit coverage proves an obsolete workflow.- Managed exactly-once enforcement ignores forbidden progress/status surfaces. Once a
chat sendorchat askexists,visibleOutputTextsdrops model progress output, andisChatAuthoringArgvexcludeschat update; a note can appear in progress/current-state text and then again in the final body while the metric still counts one.
There is one additional managed-parity gap: when no chat send/chat ask is observed, visibleOutputTexts falls back to native model output, and casePassed imposes no managed transport requirement. A managed case can therefore pass even though the note was never delivered in the required final First Tree chat send (or blocking chat ask). Please keep BYO native-final grading separate from managed grading and require the managed addressed transport; for the unresolved-conflict case, also distinguish the required blocking question from a plain send.
I did not run tests or QA, per the review-only scope.
yuezengwu
left a comment
There was a problem hiding this comment.
Re-review outcome: request changes
The new head closes the managed progress/status visibility gap, requires the final managed send/ask, distinguishes a conflict ask, and now recognizes context route → context snapshot as the required BYO sequence.
One authority-boundary blocker remains: the evaluator requires the current route/snapshot calls but does not reject an additional legacy explicit-Team tree read call. The shim still services tree read successfully, while isReadActivationArgv, readActivationCalls, and byoReadSequenceOk simply ignore it. A run can therefore execute tree read --team ... before SCOPE routing, then perform the expected route/snapshot sequence and pass every current metric. That violates the shipped skill's SCOPE-first rule and can expose target Tree content before the locally authorized candidate is selected.
Please fail the BYO case on any legacy tree read activation (or make that shim path fail for this suite), and add a regression containing both the legacy call and the otherwise-valid current sequence. The existing “legacy-only” test does not cover this bypass.
No tests or QA were run; this re-review inspected only the new diff.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: the follow-up fixes the command names and observes managed progress, but the BYO shim still validates an impossible
context snapshotinvocation and the managed transport model remains coupled to impact-note presence/effect rather than the task's delivery requirement.
Risk level: B-low
- Path baseline: C — shared compatibility comments, Web preview, skill payload, and skill-eval files.
- Semantic lift: C → B-low — this versions and rolls out a cross-consumer skill-output contract.
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: managed First Tree users and BYO coding-agent users currently receive different evidence that Context Tree decisions influenced an answer.
- Approach: replace new metadata receipts with one compact, localized, exact-version Markdown note while preserving legacy receipt readers; the latest patch attempts to align the behavior gate with current BYO and managed transports.
- Impacted modules:
skills/first-tree-read,packages/skill-evals, shared legacy receipt schema comments, and the Web preview fixture.
Review findings
❌ 1. The BYO gate now recognizes context snapshot, but the shim and its new unit event require --snapshot <path>. The shipped command accepts only --candidate and creates its own private temporary snapshot; the skill shows that exact invocation. Consequently a compliant model calling first-tree --json context snapshot --candidate ... still exits 2 in the shim, while the passing test exercises an option the real CLI rejects. Please give context snapshot a current-command shim path that allocates/returns its own snapshot, keep legacy tree read --snapshot separate if it still needs coverage, and validate the route/snapshot arguments against their real interfaces. [packages/skill-evals/src/core/shims/first-tree.ts:172]
❌ 2. Managed transport is still optional whenever impactNoteExpectation.mode === "absent". That leaves the managed navigation/no-impact trigger able to pass from native model output without any addressed chat send/chat ask—the same managed-delivery gap raised in the prior review, now merely narrowed to non-material reads. Please require a successful final managed transport for every managed trigger case, independently of whether an impact note is expected. [packages/skill-evals/src/suites/first-tree-read/metrics.ts:689]
❌ 3. The required transport kind is inferred directly from the effect (conflicted always means ask). The skill contract is conditional: append to a blocking question if the task correctly ends in one; First Tree should open an ask only when the agent's next step actually depends on a human decision. A terminal conflict assessment can therefore be a valid chat send, but this gate will reject it. Model the managed transport expectation explicitly per case/task instead of deriving it from the impact effect. [packages/skill-evals/src/suites/first-tree-read/metrics.ts:690]
Action taken
- Submitted request changes on the updated head.
yuezengwu
left a comment
There was a problem hiding this comment.
Re-review outcome: request changes
The new head closes the legacy/current BYO mixed-sequence bypass, gives context snapshot its real candidate-only private-snapshot interface, requires managed transport for no-impact trigger cases, and makes transport kind an explicit case contract.
One blocker remains in that explicit case mapping: tree-conflict-chinese is still declared with managedTransport: "ask", but its prompt only asks for a terminal assessment of whether release is allowed and says to report the unresolved conflict. The agent has no pending next step that depends on the human choosing a tradeoff. Under the managed runtime contract, importance or an unresolved product conflict alone does not justify a tracked blocking question; this case should finish with chat send. As written, the behavior gate forces a manufactured chat ask and rejects the correct terminal response—the exact coupling problem the explicit transport field was meant to remove.
Please set this case to send, or change the prompt so it genuinely asks the agent to continue with an action that cannot proceed until the human selects a tradeoff. Keep the generic unit coverage proving that a separate, genuinely blocking conflict case can explicitly require ask.
No tests or QA were run; this re-review inspected only the new diff.
yuezengwu
left a comment
There was a problem hiding this comment.
Re-review outcome: approve
The latest commit correctly changes the terminal Chinese conflict case from a tracked blocking ask to the required final send and updates the floor assertion. This closes the last outstanding review blocker.
Across the follow-up commits, the PR now enforces the current SCOPE-first context route → context snapshot workflow, rejects mixed legacy activation, models the real private snapshot interface, observes managed progress/status output, requires final managed delivery even without an impact note, and keeps send/ask selection explicit per task.
No core data structure or database change is present. No tests or QA were run during review.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: approve
- Rationale: the latest commit correctly delivers the terminal conflict assessment with
send, closing the final outstanding behavior-gate blocker without weakening coverage for genuinely blocking questions.
Risk level: B-low
- Path baseline: C — shared compatibility comments, Web preview, skill payload, and skill-eval files.
- Semantic lift: C → B-low — this versions and rolls out a cross-consumer skill-output contract.
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: managed First Tree users and BYO coding-agent users currently receive different evidence that Context Tree decisions influenced an answer.
- Approach: replace new metadata receipts with one compact, localized, exact-version Markdown note while preserving legacy receipt readers; the behavior gate now models the current SCOPE-first BYO workflow and task-specific managed delivery.
- Impacted modules:
skills/first-tree-read,packages/skill-evals, shared legacy receipt schema comments, and the Web preview fixture.
Review findings
✅ 1. The latest head maps the terminal Chinese conflict assessment to chat send, while retaining separate generic coverage for a genuinely blocking conflict that explicitly requires chat ask.
✅ 2. The follow-up commits collectively close the previously identified gaps: current route/snapshot command shapes, rejection of mixed legacy activation, managed progress/status observation, and required final managed delivery for both material and non-material reads.
Action taken
- Approved.
Why
Context Tree decision influence currently depends on First Tree message metadata and a First Tree-specific renderer. External BYO agents cannot show the same value, and First Tree Chat can present a different experience from the coding agent where the work actually happens.
The reader skill should make the influence visible in the authored result itself, with the same compact and inspectable shape everywhere.
What
contextDecisionmetadata producer contract infirst-tree-readwith one visible final-response note shared by managed First Tree Chat and BYO coding agents.0.6.0, update its interface metadata, and add behavior-level eval coverage for material/non-material use, managed/BYO parity, exactly-once output, Chinese conflict handling, root-node labels, duplicate-title disambiguation, multi-source output, exact-version links, and metadata absence.This PR intentionally does not modify or optimize the existing metadata UI. Existing stored receipts and their server/Web compatibility remain unchanged; the shipped reader stops producing new ones.
Verification
python3 scripts/quick_validate_skill.py skills/first-tree-readpnpm --filter @first-tree/skill-evals eval:floor -- --suite first-tree-readpnpm --filter @first-tree/skill-evals testpnpm --filter @first-tree/skill-evals typecheckbiome checkgit diff --checkreact-markdown; it produced one blockquote with three hard-broken lines, bold title/source, and an exact-version linkThe model-backed gate cases are declared and graded but were not executed in this change because the repository requires explicit human authorization for live model evals.
pnpm checkis not clean on currentmainbecause of pre-existing cron/client diagnostics outside this diff; every changed file passes Biome independently.Rollout boundary
This is a skill payload change. It requires no server schema, database, Web, or provider-adapter change. Consumers adopt it with the next shipped skill payload; the note uses ordinary Markdown in both managed and BYO final responses.
During rollout, agents still carrying
first-tree-read0.5.x may continue to emit the structured metadata receipt and show the existing First Tree card, while 0.6.0 agents emit the prose note. The two forms can coexist in one chat and do not duplicate within one response. The prose note is author-attributed content, not a server-authenticated sender claim or independently verified causality; a paired Context Tree change records that trust boundary.