Skip to content

Learn what a directory policy should be, without learning the wrong tree - #91

Merged
OsherElhadad merged 3 commits into
eranra:mainfrom
OsherElhadad:pr/path-lane
Sep 4, 2026
Merged

Learn what a directory policy should be, without learning the wrong tree#91
OsherElhadad merged 3 commits into
eranra:mainfrom
OsherElhadad:pr/path-lane

Conversation

@OsherElhadad

Copy link
Copy Markdown
Collaborator

What this closes

Issue #83. Write and Edit clusters refused as no-matcher-shape at src/policy/propose.ts:89, so
file-path policy could only ever be hand-written. They now have a shape and a lane.

Base origin/main (fd87f2a). No dependency on ss/learned-yellow — the gap lane it needs is
already merged.

The shell matcher does not transfer, and the reframing matters first

The issue's motivating example is "never edit anything under infra/prod/", which is a red. This
pipeline never proposes a red or an orange — proposing a safety clause from the absence of one
manufactures a deny from silence (§4.7) — so that clause is not what this lane learns, and saying so
up front avoids a reviewer looking for it. What it learns is the two levels the lanes can produce:

  • a green from the fail-closed / classifier-decided lane — "allow writes under infra/prod/
    without a classifier round-trip"
    ;
  • a yellow with no fix from the gap lane — "ask a human about writes under infra/prod/", which
    can only withhold an allow a learned green would have granted.

With that fixed, every one of the shell lane's five non-widening arguments had to be rebuilt, and each
analogous question has the opposite answer.

The unit is a segment, not a word

commandMatcher ends its literal on (?=[\s"\\]). That character class says nothing at all about /,
so it would have matched infra/production-notes/ and infra/prod.bak/ for a literal infra/prod.
The path matcher's boundary is a literal /:

Match: `/"file_path"\s*:\s*"/w/api/infra/prod(?=\/)/`

Requiring the / also means the directory entry itself is not a file under the directory. The left
anchor is the shell lane's, unchanged in spirit: the literal starts at the first character of the path
value, keyed on the tool's own argument name — so no left slack, and a Write's content (which
haystackFor includes for a red clause) cannot satisfy a green path clause by merely mentioning the
directory.

Widening runs the other way, so the floor refuses instead of falling back

For a command, a longer argument list is the widening, so E4 shrinks a token at a time until every
segment accepts the prefix. For a path a shorter prefix is the widening, and a directory rule's
natural form is a prefix — so there is nothing to shrink towards. commonPathLiteral refuses below
the floor rather than narrowing onto it, the same shape as E8: dropped, not narrowed.

The floor is two path segments below the recorded cwd (PATH_FLOOR_SEGMENTS). It is the same
number as canonicalSegment's two tokens, for the same reason: one token is a whole tool (git,
npm) and clusters everything it can do into one rule; one segment is a whole top-level tree (src,
docs, infra) and does the same. The rule people actually mean lives one level down — infra/prod,
not infra — exactly where the subcommand lives in the shell lane. It refuses the repository root and
a single component such as src by construction, and the cost is real and accepted: "allow writes
under docs/" cannot be learned. Erring narrow is the codebase's stated position (tierFor) and a
depth-1 grant in a monorepo is the whole tree.

The string is not stable, so there is one normaliser and it lives at the named seam

models.ts:272 already says the normalised call shape "belongs with src/policy/generalise.ts, not
here — this field is the seam it reads". So PATH_TOOLS and normalisedPath are there, not in
mine.ts and not in propose.ts. There was no prior path normaliser anywhere in the tree —
git grep'd for it, and generalise.ts's existing content is Claude Code rule emission, not path
handling — so this extends the designated seam rather than adding a second one.

normalisedPath requires cwd for a relative path and never defaults it: path.resolve would fall
back to process.cwd(), which is the miner's directory, and would resolve two records for the same
file to two different strings — the exact bug. A record that wrote infra/prod/net.tf and one that
wrote /w/api/infra/prod/net.tf now land in one cluster with one variant, tested end to end.

One escaper for the whole system stays true: escapeForMatcher loosens whitespace to \s+, which is
right for a command and wrong for a path (a b and a b are two directories), so a whitespace
literal is refused rather than escaped by a second escaper practices.ts asks nobody to write.

Symlinks: the cluster is refused, and the clause says it cannot resolve one

The matcher is textual, and there is no way around that: the runtime haystack is
haystackFor(tool, input), the written string. A matcher over the resolved path would never match
and would be inert; a matcher over the written string is bypassable through a symlink.

So the decision is refuse the cluster (path-symlinked) when any path in the evidence does not
resolve to where it was written. It reuses learnedClauses.ts's own realpathOf — exported for this,
including its deepest-existing-ancestor recursion for a not-yet-existing file — because two
definitions of "where does this path really go" is what makes a write boundary porous.

The comparison is of relative positions, not absolute strings. On macOS /tmp is a symlink to
/private/tmp, so an absolute comparison would refuse every candidate whose session ran under a
temporary directory. There is a test named for that.

This is a check on the evidence, at mining time, and the limit is stated where readers will hit
it: in the clause body itself, in docs/KNOWLEDGE.md under a heading that says it, and in
plugin/commands/learn.md so the agent describing a run cannot call it containment. A symlink created
after a clause is accepted is invisible to it, and assertWritable remains the only filesystem
boundary in the system.

The tool set, enumerated from the code

session.ts's PAYLOAD_KEYS enumerates the write tools by the bytes they carry, so the set is
derived rather than guessed: contentWrite, old_string/new_stringEdit, edits
MultiEdit, new_sourceNotebookEdit. NotebookEdit uses notebook_path, so the key is
per-tool and a matcher never matches a path another tool sends under another key.

Excluded, each for a reason in the code:

Excluded Why
Read, Glob, Grep, NotebookRead in tiers.ts's SAFE_TOOLS, so preClassify returns GREEN and rung 1 grants them free on every call. A learned green over them cannot change a decision (generalise.ts rule 1; replay.ts's INERT would reject it), and they can neither fall closed nor leave a gap, so neither lane has evidence about them at all.
Bash a path inside a command line is the shell lane's, and already has E8's escapesCwd.
IBM Bob's write_to_file the repo names the tool but pins its path argument nowhere. trail.ts's pick('path') is a display fallback tried across every tool, not a claim about Bob's schema. A matcher over a guessed key matches nothing forever and reads exactly like a clean run — the failure shape this wave is hunting. Add it with a real record in hand.

Three axes deliberately absent, because nothing can reach them

PATH_NEVER_WIDEN has two entries, and the test constructs a real cluster for each. The tempting
third, fourth and fifth would have been branches no honest test could trigger — the
unreachable-rule failure the process notes warn about twice:

  • traversalnormalisedPath runs path.resolve, so a .. is collapsed before the axis table
    is consulted, and a .. that leaves cwd has no shape at all.
  • out-of-cwdcanonicalPathSegment returns '', so the cluster refuses as
    no-matcher-shape before an axis is reached.
  • a secrets axis for .env, id_rsa, credentialstiers.ts's DESTRUCTIVE already matches
    those against a haystack containing file_path, so rung 1 returns RED and the record is a deny.
    A deny is neither green support (E3b) nor gap support (decision === 'none'), so it can never
    reach a support set. Covered, without a rule pretending to cover it.

What is there: corpus-path (a machine may not grant itself the corpus) and dot-root (a dot
directory at the repository root is tooling, config, or another agent's state — .git, .github,
.claude — and a learned blanket allow over it is not something six writes should buy; src/.generated
is ordinary and passes).

Two checks deleted because mutation proved them redundant

Both were written, both looked load-bearing, neither failed a test when removed — so both are gone,
with a comment saying so:

  • a symlink check on the emitted literal. It is a common prefix of every supporting path, so a
    symlinked literal is a symlinked every path and the per-path loop refuses first. Removing it
    failed nothing.
  • an isAbsolute(p) guard in commonPathLiteral. A relative path resolves against the miner's cwd,
    which is never under the session's root, so the .. test already refuses it.

Removing the per-path loop did initially fail nothing either — because the only symlink test at
the time had the whole infra/prod directory symlinked, which the literal check also caught. That is
a silence reading as success, so a second test was added for the case only the loop can see: a
supporting file escaping through infra/prod/iam while the literal infra/prod is genuinely clean.
The loop then had exactly one test, and kept it.

The output contract is unchanged

learned/<id>.md at status: proposed, through assertWritable and writeClause, dateless
<kind>-<slug>-<shape12> id, and the status guard refusing to overwrite any file whose parsed status
is not proposed. Asserted end to end, including that a declined file suppresses re-proposal.

Ledger accounting: path-below-floor joins no-matcher-shape and prefix-too-short in
suppressed.proseOnly — the shape is real and a human could still write the rule. path-symlinked
deliberately does not: that cluster is refused because the tree it describes is not the tree it
looks like, which is not advice to hand a human. Both directions are tested.

Numbers

Test files Tests
origin/main (fd87f2a) 106 2,722
this branch 107 2,764

make check green. bash ci/check-plugin-lib.sh green after make plugin, committed. eslint clean
(one pre-existing warning in SessionManager.test.ts, untouched). Zero runtime dependencies added,
Node stdlib only, no .py anywhere.

Every fixture invented; no real path or project name.

Mutation evidence

Eleven mutations, each reverting one half of the change independently, run against the committed tree
(496 policy tests as the denominator):

Mutation Failed
segment boundary (?=\/) → the shell lane's (?=[\s"\\]) 4 — every matcher-boundary case
drop the key anchor, leaving a bare directory substring 6 — including the vendored-copy left-slack case
PATH_FLOOR_SEGMENTS 2 → 1 7 — the floor, the shape depth, and the ledger accounting
segmentsOf takes the raw file_path 1 — the relative-and-absolute clustering test, and only it
drop the per-path symlink check 3 — the below-a-clean-literal case, the whole-tree case, the ledger
character-wise instead of segment-wise common prefix 11
keep the basename instead of taking dirname 5
drop the corpus-path axis 2
drop the dot-root axis 1
allow a whitespace literal 1
drop the .. out-of-cwd guard 2
drop path-below-floor from proseOnly 1

Reviewer's shortest path

  1. src/policy/propose.tspathMatcher (the boundary), commonPathLiteral (the floor and its
    inverted direction), symlinkEscape (relative positions, and why), PATH_NEVER_WIDEN (and the
    three absent axes).
  2. src/policy/generalise.tsPATH_TOOLS and normalisedPath, and the excluded-tool table.
  3. src/test/policy/paths.test.ts — every invariant, with infra/prod vs infra/production-notes/
    asserted against a compiled matcher over a real haystackFor string rather than against pattern
    text.
  4. docs/KNOWLEDGE.md — the textual-guard section, which is the thing File-path policy cannot be learned — Write/Edit clusters refuse as no-matcher-shape #83 asks the docs not to let a
    reader misread.

Not done, and deliberately

  • No relative-form matcher. One could be anchored safely at the value start, but infra/prod is
    relative to whatever cwd the call had, so it would license the same directory name in another
    repository. A relative-encoded call therefore misses the fast path and falls through to the
    classifier — under-matching, which is the fail-closed direction.
  • No change to haystackFor. Normalising at the runtime seam would change what every hand-written
    clause matches, for a gain the fail-closed fall-through already covers.
  • The floor is a constant, not a setting. The issue says "configured floor"; nothing in the repo
    consumes such a setting, and the brief's own rule is that every field must have a consumer in code
    or it does not ship. It is one exported constant with the argument for its value beside it.

Osher Elhadad added 2 commits September 4, 2026 20:44
`Write` and `Edit` clusters refused as `no-matcher-shape`, so file-path
policy could only be hand-written. They now have a shape and a lane.

The shell lane's five-part non-widening argument does not transfer, and every
analogous question has the opposite answer, so the lane is built from scratch:
the unit is a path segment rather than a word, widening runs towards *shorter*
prefixes rather than longer argument lists, the recorded string may be absolute
or relative for the same file, and a path that looks confined may not be.

- `generalise.ts` gains `PATH_TOOLS` and `normalisedPath` — one normalisation
  seam, in the module `models.ts` already names for it.
- `mine.ts` shapes a path-carrying record by its first two directory segments
  below `cwd`, so relative and absolute records for one file cluster together.
- `propose.ts` emits an anchored matcher requiring a literal `/` after the
  directory, refuses below a two-segment floor, and refuses any cluster whose
  evidence did not resolve to where it was written.
- The clause body and `docs/KNOWLEDGE.md` state that it is a textual guard and
  not a substitute for `assertWritable`.
Copilot AI lite review requested due to automatic review settings September 4, 2026 18:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

# Conflicts:
#	plugin/lib/policy/propose.js
#	src/policy/propose.ts
Copilot AI review requested due to automatic review settings September 4, 2026 19:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@OsherElhadad
OsherElhadad merged commit 4118856 into eranra:main Sep 4, 2026
7 checks 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