Skip to content

[Architecture]: two modules both claim to own "private-looking text" #5136

Description

@JunZ-Leo

Problem / 问题

Two modules each document themselves as the canonical answer to "is this text private?", and
neither dominates the other.

  1. loopx/public_safe_text.py opens with "Canonical private-looking-text rules for public-safe
    control-plane fields"
    , explains that its rule set "used to be copied into each owner, and the
    copies drifted", and names tests/fixtures/public_safe_text_corpus.json as the corpus pinning
    the Python and TypeScript owners to one contract. Consumers: feedback, authority,
    boundary_authority, control_plane/goals/artifact_lifecycle.
  2. loopx/control_plane/runtime/public_safety.py carries an independent
    SECRET_LIKE_SURFACE_PATTERN + LOCAL_PATH_SURFACE_PATTERN, used by
    validate_public_safe_value — the guard on the busiest public-output path (12 production
    modules call it). It never consults public_safe_text.

Measured on b15413ffc, calling both owners' matchers directly. A HIT means the text is treated
as private; no row is a HIT in both columns:

value public_safe_text public_safety
ghp_… (36 filler chars) — HIT
eyJ….….… (JWT shape) — HIT
access_key=… — HIT
C:\Users\bob\x — HIT
the bare word Bearer / password / secret HIT —
the Bearer token expired HIT —
token=x (short value) HIT —
larkoffice…, docs.internal… HIT —
Authorization: Basic <base64> HIT —
see file:///Users/alice/goal.md HIT —
path:/Users/alice/goal.md HIT —

The consequence is visible in the code that sits between them: on main,
control_plane/goals/artifact_lifecycle.py writes
if find_private_text_match(value) or _TOKEN_SHAPES.search(value), where _TOKEN_SHAPES is a
private list of provider prefixes carrying this comment: "Provider token shapes the shared
private-text rules do not cover … a leaked token there must never reach a public projection just
because the shared corpus did not list its prefix."
That caller knows it needs two owners and
patched the gap locally. #5135 folds _TOKEN_SHAPES and five sibling copies into the
public_safety owner, which removes six copies but still leaves that caller OR-ing
public_safe_text with public_safety, because the prefixes are not the only thing the two corpora
disagree about.

Why this needs an owner decision rather than a merge

The two corpora encode different philosophies, and neither is obviously wrong for all surfaces.
public_safe_text rejects ordinary English words (Bearer, password, secret) — its docstring
records that it deliberately moved one rule away from the bare word because governance prose says
"owner authorization" constantly, while other rules stayed word-shaped. public_safety instead
requires an assignment shape with a length floor so that "the token budget is 1200" survives.
Picking either winner changes what the other set of surfaces is allowed to publish, and it will move
expectations in tests/fixtures/public_safe_text_corpus.json and the TypeScript mirror. There is
also a prefix-blindness dimension to settle: public_safety's path rule excludes a preceding : or
/, which is why file:///Users/alice/goal.md and path:/Users/alice/goal.md pass it while
public_safe_text catches them.

Desired outcome / 期望

One module owns "private-looking text". The other is deleted, or becomes a policy layer that only
adds per-surface thresholds and words on top of the shared shapes. No caller should have to consult
two owners for one value.

Acceptance / 验收

  • A single named owner for private-text detection, whose docstring states which rules are
    word-shaped and which are shape-with-threshold, and why.
  • artifact_lifecycle no longer ORs two owners for one value.
  • The shared corpus fixture pins both directions of the table above, so a future consolidation
    cannot silently drop a rule that only one side had.
  • file:///Users/… and path:/Users/… are either rejected by the surviving owner or recorded as
    accepted, with the reason.
  • feedback / authority / boundary_authority and validate_public_safe_value are shown to
    reach the same verdicts on a named subset, or each surface's deliberate difference is written
    down next to the owner.

Activity

  1. huangruiteng commented on Sep 27, 2026

    @huangruiteng
    Collaborator

    这里有个细节,开发代码提pr时的public safe比loopx内部状态的public safe要严格一些,后者可以松一些

  2. karenchuu commented on Sep 27, 2026

    @karenchuu
    Contributor

    PR #5196 closes the half of this that needed no decision: the raw-remote-location shape was compiled identically in three private names while the public_safety owner had no counterpart for it. That list is now owned once and the three sites import it; each site keeps its own error text and thresholds, and the scheme coverage is unchanged (ftp stays outside, pinned by a test so nobody widens it inside a deduplication).

    The other half — which of the two owners wins the private-looking text philosophy — I did not resolve, and here is the measurement that decision needs. Four dialects of "this string carries a local path" are live on main, and every one is stricter than the others somewhere:

    input public_safety owner periodic_report.core copy packets / _validation copy ml_experiment copy
    /Users/x/a.py rejects rejects rejects rejects
    ~/notes.md misses misses rejects rejects
    path:/Users/x/a.txt misses rejects rejects rejects
    /etc/passwd rejects misses misses rejects
    /var/folders/ab12/tmp/x rejects rejects misses rejects
    C:\Users\x\a.txt rejects misses misses rejects
    \\srv\share\a (UNC) rejects misses misses misses
    see /private/tmp/x now rejects rejects rejects rejects

    Three things follow that are yours to weigh rather than mine to assume:

    1. The owner is the only dialect that sees a UNC share, and the only one that rejects /etc/passwd — but it misses ~/ and a colon-prefixed absolute path, both of which some callers already reject. So a merge is not a no-op for any of the four validators; it is a union that tightens four shipped entry points at once.
    2. ml_experiment looks broad in the table because its rule is a different decision shape: it rejects any value starting with / or ~, plus a drive letter. Folding it into a surface-scan union would silently drop that leading-position rule unless it is preserved as its own predicate.
    3. periodic_report.core carries /var/folders explicitly, which reads like a macOS-bug workaround rather than a policy. If it is a workaround, the owner should record why, not inherit it.

    Say the word if you want the union executed here: monotonic tightening, per-site before/after lists of newly-rejected inputs, a literal-scan guard so no fifth dialect appears, and I would keep each site's message and length thresholds exactly where they are.

  3. huangruiteng commented on Sep 27, 2026

    @huangruiteng
    Collaborator

    Thanks for separating the mechanical deduplication from the policy decision. The direction for #5136 is one shared detection contract, with explicit policies for the destination/field. Please proceed on that basis, rather than applying a blanket union to every validator.

    To clarify my earlier comment: repository/PR publication and owner-private operational state have different disclosure boundaries. A field does not become private merely because it is stored inside LoopX; existing public-safe projections must continue to enforce their publication boundary.

    1. Consolidate ownership. Use the public_safe_text contract as the shared home for text classification; runtime/public_safety should consume it for recursive payload validation and public-output policy, rather than own a competing set of text shapes. Keep Python/TypeScript semantics pinned to one corpus, following the existing typed-owner direction. Return explicit categories/reasons so callers can apply a named policy. artifact_lifecycle should make one policy-aware call, without OR-ing independent detectors. This does not require a new capability or a broad language migration.

    2. Separate detection from permission to publish. Recognizable credential values, authentication headers and private-key material must remain blocked from ordinary state summaries and public output; the internal policy is not a credential-storage exemption. Bare words such as Bearer, password, and secret, or “the Bearer token expired”, are not by themselves evidence of a leaked credential. Short assignments such as token=x need an explicit, tested policy decision, not accidental acceptance because one regex has a length floor. Organizational markers, private links and local paths are a different category: owner-private fields may retain necessary references where their contract allows it; public output must reject or redact them. Generic detection should not equate every HTTP URL or vendor name with private information. Alias-only fields can retain their stricter field contract.

    3. Close path-detection gaps without silently changing every consumer. The shared classifier should recognize home-relative paths, Windows drive/UNC paths, and local paths behind file:// or path: prefixes. Public-safe output should reject/redact those local references. Preserve ml_experiment’s leading / or ~ rule as an explicit alias-field constraint, rather than losing it in a substring scan. Recognizing a location does not imply that every internal field must reject it. Export must revalidate for its destination; an internal acceptance result cannot serve as a public-safe receipt.

    4. Make the migration reviewable. Add a compact caller/field → policy table and corpus cases covering both issue tables, ordinary prose, and relative aliases. Test the real caller entry points, nested payloads, Python/TypeScript parity, and internal-to-public export. For each migrated surface, report newly accepted and newly rejected cases; unchanged surfaces need parity evidence. Deliberate removal of a bare-word false positive is a behavior change and should be documented as such. Preserve local messages and size limits unless separately justified. A source-level duplication guard can supplement these tests, but cannot establish semantic correctness.

    #5135 is merged; #5196 is still open and can remain scoped to its behavior-preserving raw-location extraction. This issue stays open for the policy-aware consolidation and caller migration above. The acceptance target is a single owner with justified differences between surfaces, not identical verdicts for private storage and public publication.

  4. karenchuu commented on Sep 28, 2026

    @karenchuu
    Contributor

    Starting on direction 1 with a behavior-preserving consolidation PR: #5245.

    That replaces #5241, which I am closing: the same change, re-landed so this issue's thread stays under one account.

    What it does: loopx/public_safe_text.py becomes the single home for the
    "does this string look private?" decision. control_plane/runtime/public_safety.py
    now consumes SECRET_LIKE_SURFACE_PATTERN, LOCAL_PATH_SURFACE_PATTERN and
    REMOTE_LOCATION_SURFACE_PATTERN from there instead of owning a competing set,
    the classifier returns an explicit category plus reason, and artifact_lifecycle
    makes one policy-aware call rather than OR-ing two independent detectors.

    Deliberately held out of it, so the review can stay a pure relocation:

    • the bare-word and short-assignment changes ("the Bearer token expired",
      token=x) — that is a behavior change, so it goes in its own PR;
    • enforcing the newly recognized ~/ and path: local-path shapes on any
      surface. Recognition is behind include_path_gaps, default off, so nothing
      tightens until you decide which surfaces should.

    One decision I am not making unilaterally, because it changes what a published
    projection may carry: is a file:// URL a remote location or a local path? It
    currently belongs to REMOTE_LOCATION_SURFACE_PATTERN, so artifact_lifecycle
    accepts file:///Users/dev/model.bin while decision_context,
    material_lifecycle and ml_experiment each reject it at their own thresholds.
    Reclassifying it as local would tighten artifact_lifecycle, which is why it is
    not in this slice — say which way you want it and I will land it as a disclosed
    change with the per-surface newly-rejected report.

    For the reviewer-gated evidence you asked for: the credential-shape and
    remote-location owner guards now name public_safe_text.py as the single owner,
    and the semantic inventory counts are identical before and after the move. I am
    also putting the caller/field -> policy table and the per-surface newly
    accepted / newly rejected report in a follow-up.

  5. karenchuu commented on Sep 28, 2026

    @karenchuu
    Contributor

    Correction to my previous comment: I raised "file:// — remote location or local path?" as an open question, but direction 3 already settles it. It asks the shared classifier to recognize "local paths behind file:// or path: prefixes" and says "Public-safe output should reject/redact those local references." So the answer is: local path, and a public projection must stop carrying it. I am not waiting on that decision.

    Reading your comment again against the shipped code, three things in that direction are not true today, which is why landing it is a disclosed behavior change rather than part of the relocation in #5245:

    • LOCAL_PATH_SURFACE_PATTERN deliberately excludes a preceding : or /, so file:///Users/… and path:/Users/… are not rejected by public-safe output today; they only fell out because the separate raw-location shape rejected any file:// URL as a remote location.
    • artifact_lifecycle has always let an ordinary http(s) URL through, so any rule that treats file:// as a local path has to be stated and tested against that surface instead of arriving as a side effect.
    • The ~/ and path: recognizers I added in refactor(control-plane): single owner for private-text classification #5245 are behind include_path_gaps, default off, so they currently enforce nothing.

    What I intend for the follow-up, with each decision named and tested rather than inherited from a regex accident:

    1. Public-safe output rejects/redacts ~/…, path:/…, file:///… and Windows drive/UNC local paths — the tightening you described, with a per-surface newly-rejected list, since it moves four shipped entry points at once.
    2. Bare words Bearer, password, secret and "the Bearer token expired" stop being credential evidence, per direction 2. This changes the verdict of feedback, authority, boundary_authority, the TypeScript Vision checkpoint and artifact_lifecycle, so the corpus fixture and the TypeScript mirror move with it in the same diff, and I will report which corpus samples change side.
    3. token=x gets an explicit threshold policy instead of acceptance-by-length-floor: assignment shapes count as credential evidence only when the value reaches a named minimum length, that minimum is a stated constant with a test on both sides of it, and the prose that survives ("the token budget is 1200") is pinned. I picked the accept-short side because provider credentials are long and a sub-threshold value is a placeholder, but this is the one row where I would rather you say which way, since it is a published-projection boundary.
    4. ml_experiment's leading /-or-~ rule stays its own alias-field predicate, as you said in point 2 of the dialect table; refactor(control-plane): single owner for private-text classification #5245 pins it with a test showing the shared classifier does not flag what that field rejects.

    I will also fold in the caller/field → policy table, the nested-payload and internal-to-public export cases, and per migrated surface the newly accepted / newly rejected report, with parity evidence for the surfaces that stay unchanged.

  6. gcl-coder commented on Sep 29, 2026

    @gcl-coder
    Contributor

    Direction 3, local-path half: delivered as PR #5296 (author gcl-coder).

    decision_context/packets.py:43 and material_lifecycle/_validation.py:18 each
    still enforced "does this text carry a local path?" with their own identical
    regex, while the credential and raw-remote-location rules right next to them
    already came from the shared seam. Both now answer through
    public_safe_text.find_public_safe_local_path, which is the absolute-root
    pattern plus the two gap shapes #5245 left opt-in plus a colon/equals boundary
    arm, so recognition covers home-relative ~\…, path:-prefixed, drive-letter
    and UNC references and the roots the copies simply did not list. Measured over 51
    samples including every rendered row of the shared corpus: 17 more values
    rejected, none newly accepted; the PR also enumerates 616 boundary x root x
    position combinations and asserts the one-directional property.

    Three things I did not decide for you, and why:

    • file:// is not recategorized. Both migrated sites already reject every
      file:// value, through the remote-location rule they import, with their own
      wording — so no observable verdict changes, and moving the category only
      changes which reason a caller sees unless a surface starts accepting ordinary
      URLs while rejecting file://.
    • validate_public_safe_value is untouched. It has 14 product call sites with
      different destinations, and your direction 3 rejects per destination. I did
      not measure how many would newly reject. A test pins its current narrower
      verdict so the later pass has to change it deliberately.
    • extensions/presentation.py:49 is a third copy, but not the same decision in
      either direction, and that is measurable rather than assumed: it rejects
      //Users/alex/x and ///tmp/a.log, which the owner's negative lookbehind
      does not match, while it accepts x=/private/a, which both migrated sites
      reject. So a drop-in migration would loosen it.

    Question for you, one line is enough: should the owner recognize a run of
    slashes before a root (//Users/…, ///tmp/…) the way presentation.py does?
    If yes, that third surface can move onto the owner in a follow-up and the tree
    loses another copy; if no, presentation.py keeps its own rule and the reason is
    recorded in the PR rather than left to be rediscovered.

    registry.py:89 is left alone for a different reason: combined with
    Path(text).is_absolute() and placeholder skipping, it answers "may this path be
    recorded in the registry", not "may this text be published".

  7. sakurahello1 commented on Sep 30, 2026

    @sakurahello1
    Contributor

    Refs #5136, directions 2 and 4. Claiming the bare-word false-positive half plus the per-face strictness that direction 2 already settles; I am not deciding the short-assignment question raised in this thread on 2026-09-28 — token= keeps its current verdict at any value length, now under a named arm with tests on both sides.

    One rule enforced the whole contract: an 11-pattern tuple shared by feedback, authority, boundary_authority and the TypeScript Vision checkpoint. It rejected the words bearer/password/secret wherever they appeared, and it had no arm at all for a credential value that arrives without a label. So both halves were wrong in opposite directions:

    • an operator note reading the Bearer token expired was refused as a leak;
    • ghp_ + 36 characters written bare into the same field was accepted, because the rule only recognized a token next to an Authorization: label.

    This change separates the two. The bare words move to their own category, which the four internal-state owners recognize but do not reject; the value and assignment shapes stay in every policy, and the owners now reach the shared shape detectors they never had. Measured over the 972-form class of prefix × word × separator × value: 536 word-only forms the old rule rejected are now accepted by these owners, and no form carrying an assignment or a scheme value is among them — that is asserted as an invariant over the whole class, not over a sample.

    Two things I deliberately did not do:

    • URLs are excluded from the internal-state policy rather than decided. The rule these owners enforced never rejected an ordinary link, so migrating them onto the classifier would have started to. That is per-face work, listed as remaining.
    • token=x short assignments keep their verdict. Direction 2 says that half needs an explicit, tested policy; it does not say which way. The arm is now named and tested on both sides so the decision is visible when you make it.

    The TypeScript owner gains the two in-policy shape arms, ported from the same compiled pattern sources rather than retyped, so one corpus yields one verdict in both runtimes (direction 4's same-corpus requirement).

    Corpus: tests/fixtures/public_safe_text_corpus.json gains a third bucket, internal_state_prose, for what the four owners now accept while the publication tier still rejects. The differential is pinned as two named lists against a frozen copy of the previous rule, so the accepted loosening and the accepted tightening are both in the test, not only in this description.

  8. AronSwan commented on Sep 30, 2026

    @AronSwan
    Contributor

    Windows follow-up: the scheme-list sentinel added with the single-owner refactor (#5245, moved to tests/control_plane/ in #5270) can never pass on Windows. str(path.relative_to(REPOSITORY_ROOT)) yields \ separators there, so the offender never equals the asserted POSIX literal loopx/public_safe_text.py.

    All substantive tests in the file pass on Windows; only the literal-scan drift sentinel is permanently red — which both disables the guard's signal for Windows dev loops and invites alert fatigue. CI runs Linux, so the merge didn't surface it.

    One-line fix (.as_posix(), identity on POSIX): #5356

  9. sakurahello1 commented on Oct 7, 2026

    @sakurahello1
    Contributor

    Following up on my earlier #5335 contribution, I am taking a bounded direction-3 migration: the typed public-output validator and its existing callers still accept home-relative references, explicit path-prefixed references, and file URLs. A persisted run can consequently contribute a local locator to the public Goal artifact-lifecycle projection.

    I have reproduced the gap on main and am fixing it through the existing shared local-path owner, covering recursive payload keys/values and real status readback. Ordinary remote URLs remain governed by each destination's existing policy. Internal text-owner policies, the generic compactor and presentation redaction are outside this slice.

    I will include the affected-caller policy map, regression evidence and diagnostic compatibility changes in the PR. This addresses one remaining boundary in #5136, not the whole parent issue.

  10. Hsuehtan commented on Oct 7, 2026

    @Hsuehtan
    Contributor

    Refs #5136, directions 1, 2 and 4. Delivered as PR #5876 (Hsuehtan).

    decision_context/packets.py:50 and material_lifecycle/_validation.py:25 were still keeping their own credential alternation list — byte-identical to each other, consulted as SECRET_LIKE_SURFACE_PATTERN.search(text) or _CREDENTIAL_RE.search(text), and commented as if it were a threshold. Both now make one categorized call against a named CREDENTIAL_CATEGORIES policy, with the local message and length limit kept per face.

    Two things the direction notes did not predict, measured rather than assumed:

    • One spelling the deleted lists reached and no owner category arm reaches: a credential label glued into a field name (db_password = "…", password_hash=…). Every label arm anchors with \b, and _ is a word character, so those were invisible to the owner. It is covered by a new owner arm reached only through an opt-in flag, default off, so the four migrated text owners and the publication tier change no verdict — asserted in a test rather than argued.
    • The delta runs both ways over a 2,147-input grid: 180 spellings are now refused that were not (assignment arms with no length floor, the = spelling of an authorization header, a bare scheme word), 741 are accepted that were refused — every one of them a label with nothing adjacent stating an assignment, plus a PEM header written without its dash fence. Both lists are pinned as named assertions.

    Not moved, and why: remote_location / local_path at these faces (still asked separately, unchanged — that is the per-face caller migration #5136 stays open for), and the short-assignment decision, which direction 2 asks for and does not state. A census guard added here names the three faces that still decide this question for themselves — extensions/presentation.py, control_plane/todos/handoff_note.py, loopx/contract.py — each declared with its reason; presentation is blocked on exactly the short-assignment item above. Three more credential-ish rules (env-name, field-name, registry marker) are deliberately not flagged: they decide a name, not whether text carries a credential value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions