fix: make Context Tree value audits evidence-safe - #4
Conversation
yuezengwu
left a comment
There was a problem hiding this comment.
Blocking: AGENT_SLUG_PATTERN at skills/context-tree-insights/scripts/context_tree_insights.py:82 does not cover the full grandfathered producer grammar that this patch promises to preserve.
Before the current 1–64 rule, First Tree created names with ^[a-z0-9_-]+$ up to 100 characters. Current First Tree explicitly grandfathers rows outside the tightened regex, so an existing runtime slug such as _legacy or -legacy can still have a valid local binding and can still be passed to chat --agent. This new gate rejects both before verify_cli_agent_identity() can cross-check the binding, leaving those still-runnable Agents unable to run the audit.
Please accept the complete historical 1–100 producer grammar in addition to the current grammar (the local binding UUID check still provides the fail-closed identity guarantee), and add leading _ and - cases to the grandfathered compatibility matrix. The current test that classifies -fixture-agent as invalid should be updated accordingly.
|
Fixed in the new head.
Validation: repository Skill validator, compileall, 19/19 deterministic tests, |
yuezengwu
left a comment
There was a problem hiding this comment.
The requested compatibility fix is complete at af3bd18.
The collector now accepts the full historical [a-z0-9_-]{1,100} producer grammar, including leading separators, while retaining the runtime/workspace/local-binding UUID equality check as the fail-closed identity authority. Agent selectors are consistently passed as --agent=<slug>, so option-looking grandfathered names remain data rather than CLI flags. The updated regression matrix covers the reported cases, and I found no new blocking issue in the incremental or full diff.
yuezengwu
left a comment
There was a problem hiding this comment.
The runtime adapters and Task-level pending semantics are moving in the right direction, but the new evidence grammar still has three fail-open / non-conserving cases that block an evidence-safe release:
-
safe_tree_cli()only searches for adjacenttree treetokens anywhere in the argument vector (skills/context-tree-value-audit/scripts/context_tree_value_audit.py:1943). As a result, a different First Tree namespace such asfirst-tree chat send tree treeis accepted as a read-only diagnostic; when it appears in a compound command with a real Tree Markdown read, the whole call can be classifiedaccepted_read_only_compositeeven though it performs an external mutation. Please parse/whitelist the actualtree treecommand path (and only its safe selector arguments), rather than accepting those words at arbitrary positions, and add a regression for a mutating namespace containing the same token pair. -
The
rgpath treats every unknown option as harmless and only validates bare path-shaped tokens (...py:2028,...py:2167). Options that read their values from files therefore bypass Tree isolation: for examplerg --file=/etc/passwd pattern <tree>/NODE.md(and similarly--ignore-file=/...) can recover the Tree operand and be accepted while the original process also reads outside the bound Tree. This contradicts the documentedrejected_unsafeboundary for proven Tree-external reads. Please whitelist the supportedrggrammar and validate/reject every file-valued option, including--option=valueforms, with external-path regressions. -
Claude call/result pairing drops incomplete calls before classification (
...py:4171-4177). An in-windowtool_usewhose literal payload names bound-Tree Markdown but whosetool_resultis missing or duplicated produces a coverage-gap string, yet it never reachesprovider_read_rows(), so it contributes zero toin_window_tree_read_attemptsand none of the four statuses. The report can then claim that all such calls are conserved when the denominator omitted them. Please carry attributable unpaired/invalid calls through asunresolved_opaque(or otherwise make the denominator truly include them) and add missing/duplicate-result conservation regressions.
These are core collector/schema correctness issues; there is no database change. Per reviewer policy I reviewed the diff statically and did not run the test or QA suites.
|
Addressed all three blocking findings in
I also added regression coverage for the exact reported examples plus path-qualified reader spoofing, Validation:
|
yuezengwu
left a comment
There was a problem hiding this comment.
The previous three blockers are fixed at e7baf37: First Tree commands now require the actual tree tree namespace, file-valued/unknown rg options are closed, and incomplete Claude pairs remain in the four-state denominator. Two implicit side-effect paths still keep the new “read-only” grammars from being statically closed:
-
safe_tree_cli()allowsfirst-tree tree treewithout--no-pull(skills/context-tree-value-audit/scripts/context_tree_value_audit.py:2038-2105). The CLI’s own help states thattree treeperforms an automaticgit pull --ff-onlyunless--no-pullis supplied. Thereforecd <tree> && first-tree tree tree && cat NODE.mdis currently accepted asaccepted_read_only_compositeeven though the original call can perform network I/O and mutate the Tree checkout. Please require--no-pullfor selector executions (while handling a standalone help invocation separately) and add a regression proving the default-refresh form is rejected/unresolved. -
The closed
rgargv grammar still makes--no-configoptional (...py:2179-2242, with the flag merely listed at line 243). Ripgrep reads the file named byRIPGREP_CONFIG_PATHunless--no-configis present, and that config can inject options such as--preor--file; historical tool records here do not bind the process environment or config contents. Consequently even a barerg Decision <tree>/NODE.mdcannot prove “no external program / no Tree-external file” from the recorded command alone. Please require--no-configfor acceptedrgcalls (or persist and validate the exact historical config/environment) and cover the implicit-config boundary in the regression matrix.
Both cases directly contradict the schema’s promise that accepted composites are statically closed and that network, external-program, and external-file shapes are rejected. These remain collector/evidence-grammar issues; there is no database change. Per reviewer policy I did not run tests or QA.
|
Resolved the two follow-up blockers in
The evidence schema and manual behavior checklist now state both requirements. Validation:
|
yuezengwu
left a comment
There was a problem hiding this comment.
The two requested fixes are correctly implemented at 4d087cd: selector execution now requires --no-pull (with exact help isolated), and accepted rg calls require --no-config. One equivalent implicit-configuration execution path remains in the existing Git diagnostic whitelist:
safe_git_diagnostic() accepts git diff, git log, and git show without proving that external diff/text-conversion helpers are disabled (skills/context-tree-value-audit/scripts/context_tree_value_audit.py:2146-2183). Git’s own diff-options documentation states that textconv helpers are enabled by default for git diff and git log family commands, and repository attributes/config can name an external helper; --no-ext-diff and --no-textconv are the controls that disable those paths: https://git-scm.com/docs/diff-options. Historical tool rows do not bind the Git config, attributes, or process environment, so a command such as git -C <tree> diff && cat <tree>/NODE.md can still be classified accepted_read_only_composite while the original Git process executed an external program.
Please close the Git grammar the same way as rg: for commands capable of diff rendering, require the explicit no-helper controls (or otherwise persist and validate the exact historical Git configuration/environment), and add a regression with configured textconv/external-diff potential. Please also review the remaining whitelisted Git subcommands for other config-driven process hooks before treating the whitelist as statically closed.
This is the same collector/evidence-safety boundary, not a database change. Per reviewer policy I reviewed statically and did not run tests or QA.
|
Resolved the Git configured-helper blocker in The collector no longer accepts any Git command as a statically closed read-only diagnostic. This is intentionally stricter than maintaining a partial subcommand grammar: unbound system/global/repository/environment configuration can activate external helpers during rendering, status, and even index-backed revision resolution. The unsafe classifier now scans the full Git argv, including global
The evidence schema and manual checklist now consistently state the all-Git fail-closed policy. Validation:
|
yuezengwu
left a comment
There was a problem hiding this comment.
The Git blocker is fully closed at 99a75e2: all Git commands now fail closed instead of depending on unbound config/attributes. The same external-helper issue remains in the bat content-reader whitelist.
bat is accepted as a pure reader (skills/context-tree-value-audit/scripts/context_tree_value_audit.py:69, :1664-1803), and the parser explicitly accepts any --paging=... value, including --paging=always, without requiring --no-config or no-paging mode. Bat’s own documentation states that it loads system/user or BAT_CONFIG_PATH configuration, that config may set --pager, and that paging launches the command from BAT_PAGER/PAGER; a TTY or forced --paging=always can therefore execute an external pager: https://github.com/sharkdp/bat#automatic-paging and https://github.com/sharkdp/bat#configuration-file. The historical shell assessment does not bind the TTY flag, Bat config, or pager environment, so bat <tree>/NODE.md (and explicitly bat --paging=always <tree>/NODE.md) can still be classified as an exact/read-only Tree read while the original process launched an unbound external program.
Please either remove bat from the accepted reader grammar, or require explicit CLI controls that override both paths (at minimum --no-config plus documented never-page mode) and reject every other paging form. Add regressions for bare Bat, forced paging, and the fully closed accepted form. Please apply the same final implicit-helper scan to the remaining accepted executables before claiming the grammar is statically closed.
This is a collector/evidence-grammar safety issue, not a database change. Per reviewer policy I reviewed statically and did not run tests or QA.
|
Addressed at exact head The follow-up takes the conservative Skill-only route: historical tool rows do not seal Git's system/global/repository configuration, attributes, environment, hooks, or optional-lock behavior, so no Git diagnostic is now eligible for
Validation on the immutable commit:
Two independent exact-range reviews are in progress. The PR remains blocked; this comment does not request merge or release yet. |
|
Resolved the remaining implicit-helper and open-grammar paths in
I also performed a final broad scan of every remaining accepted executable. Two independent reviewers found no remaining pager, preprocessor, external-helper, file-valued-option, recursive-dereference, or open-argv blocker. Validation:
|
yuezengwu
left a comment
There was a problem hiding this comment.
Approved at exact head d9491d529b7823defb7ac0731ab48df7fb65ae2f.
The prior Bat blocker is fully resolved: Bat is no longer eligible for accepted read evidence, so unbound config and pager execution cannot be misclassified as a statically closed read. The same incremental range conservatively removes find and ls, requires rg --no-config --no-ignore, and closes the accepted head / tail / nl pipeline-filter grammars.
Together with the earlier fixes, the collector now preserves the four-state read-attempt denominator while failing closed on runtime identity mismatch, provider pairing anomalies, mutable/config-driven command behavior, external paths, and helper-capable Git commands. I found no new blocking issue in this incremental range or the previously reviewed full diff.
This remains a core collector/evidence-schema change, but there is no database change. Per reviewer policy, I did not run tests or QA; the approval is based on static diff review (GitHub Actions is currently green independently).
What changed
.outputforwarding with literal inner arguments, and single-branch literal filesystem guardsN/A / pending, never zero effectWhy
The original pilot exposed two independent failures. First, mutable display names and Chat summary timestamps could prevent a valid historical export. Second, the collector accepted only isolated single-file reads even though real Agents commonly read several Tree nodes through one read-only shell or orchestration call.
Those false negatives must not erase previously reviewed value evidence or turn missing exposure into a numeric zero. The collector now recovers only shapes whose commands, Tree paths, and output forwarding can be proved. Everything else stays unresolved.
Validation
python3 scripts/validate_skill.pypython3 -m compileall -q skills tests scriptspython3 -m unittest discover -s tests -v(27/27)git diff --check00711a4: 123 clear Tasks, 40 confirmed exposure, 83 pending, and 24 current-sample effect Tasks; the reviewed 37-effect baseline remained separateThe new exact head is
ed0ece52c14bec0579de1deeb78707758e5a3572. A same-window private rerun and Task-level re-judgment are in progress. This PR remains Draft until that rerun passes the evidence and release gates; pending Tasks will not be interpreted as no effect, and saturation will not be claimed without the required no-new-findings batches.