Skip to content

refactor: third-pass sink-file and boundary governance #1404

Open

Description

@Astro-Han

Context

Second-pass governance (#1084) is winding down: 10 of its slices are merged and the remaining ones are tracked as sub-issues there. This issue tracks the third pass: ~150 source files ≥350 lines (everything not already covered by #1084) audited against the same four criteria, with per-finding adversarial verification against current main. 32 findings survived verification; 18 were refuted — plausible-but-justified coupling was explicitly rejected (e.g. model-adapter.ts usage normalization, permission.ts shell classification, and app-shell.tsx mode management are all fine as-is).

The dominant pattern this round is different from #1084: 25 of 32 findings are silently-diverging parallel implementations (copy-paste that must be kept in sync by hand), not mixed responsibilities. Several copies have already drifted.

Headline evidence

  • bots/qq-bridge.ts, bots/discord-bridge.ts, and most of bots/dingtalk-bridge.ts copy the same WebSocket gateway lifecycle wholesale (opcodes, heartbeat/seq/session, connect/reconnect/backoff — the backoff constants are byte-identical). bots/simple-bridge.ts additionally carries dead Feishu/Discord paths superseded by the dedicated bridges and only actually runs Telegram.
  • Five apps/desktop/src/main/oauth/*-subscription-service.ts files copy one service shell (pending/logout/refresh/getAccessToken/failure handling); the codex and antigravity services duplicate the loopback callback server nearly verbatim. The missing base leaks downstream into subscription-ipc-main.ts (7 same-shape handlers per provider) and oauth-model-connections-main.ts.
  • Two full 5-field cron parsers exist across packages: core/plan-reminders.ts (parsePlanReminderCronExpression/cronExpressionMatches, ~L541–656) and runtime/automation-state.ts (expandCronField/normalizeCronExpression/computeNextCronFire/matchesCronField, ~L521–696) — and inside automation-state.ts the same field grammar is parsed twice with already-divergent edge handling.
  • Security-relevant: main/rive-cli.ts ships a private redaction implementation (redactRiveText/redactRiveValue, ~L160–178) parallel to @maka/core's authoritative redactSecrets, with narrower coverage (misses GitHub/Google/Slack token shapes) — a future hardening of the authoritative rules silently won't apply here.

Slices

Each High/medium checkbox is one independently reviewable, independently revertible PR unless noted; low-severity items are batched.

High

  • Bot gateway bridge base — extract GatewayBridgeBase (opcodes, heartbeat/seq/session, connect/reconnect/backoff, close policy); qq/discord/dingtalk override only auth header, identify/resume payload, dispatch event names, send route; includes the shared reconnectBackoffMs. Follow-up in the same lane: delete simple-bridge.ts's dead Feishu/Discord branches and rename it telegram-bridge to match its real responsibility.
  • Desktop subscription-OAuth base — extract a SubscriptionOAuthServiceBase (or composable helpers) for the five services; extract startLoopbackAuthCallbackServer shared by codex/antigravity; then collapse the per-provider handler blocks in subscription-ipc-main.ts and the repeated needs_reauth blocks in oauth-model-connections-main.ts. 2–3 PRs, one owner.
  • Cron unification — single cron-expression module in @maka/core; plan-reminders.ts and runtime/automation-state.ts consume it; inside it, one field-grammar parser (today expandCronField validates strictly while matchesCronField silently skips invalid parts).
  • main/rive-cli.ts redaction (security) — delete redactRiveText/redactRiveValue, use @maka/core redactSecrets (thin deep-recursive wrapper if needed).

packages/runtime

  • Compaction helper dedup — history-compact-checkpoint.ts (~L516–546) and semantic-compact.ts (~L1515–1578) re-inline primitives their own domain authority context-budget-helpers.ts already exports (estimateTokens/sha256/nonEmpty/escapeAttribute/uniqueSorted/finiteRatio); the escapeAttribute copy has already diverged (extra > escaping). Also collapse the stableStringify copies (active-full-compact-facts.ts:558 et al.) and the ~9 three-line sha256 wrappers. Coordinate with refactor: reuse shared result and hashing helpers #1374 (hash reuse, approved). Caution: the local uniqueSorted variant keeps empty strings — a contract difference to preserve or consciously resolve.
  • Sandbox-denial predicate — isLikelySandboxDenial is verbatim in shell-tools.ts:400 and builtin-tools.ts:1085, with a third variant in shell-run-tool-result.ts:169; extract one primitive over {stdout, stderr, sandboxed}.

packages/headless

  • canonicalJson — five copies (one already diverged on undefined handling); single serializer next to buildRunManifestFingerprint.
  • harbor-cli.ts connection reading — stop re-parsing workspace llm-connections.json (schema + per-platform path, ~L770–843); route through the shared workspace-path + connection reader / ConnectionStore authority.

packages/core

  • Text-sanitize pipeline — foreign-session.ts sanitizeForeignText (~L94–106) copies session-name.ts's pipeline and has drifted; extract sanitizeUnicodeText(text, {maxCodePoints}).
  • TokenUsageFields — the same token-usage field bundle is declared in three places with no shared base and has already drifted (runtime-event.ts:204–229 et al.); extract one shared interface and make isTokenUsageFields follow it.

packages/storage

  • JSONL base batch — shared readJsonlLines(path, {strict, tolerateTornTail, decode}) (today 5+ hand-rolled readers with silently divergent torn-tail policies); shared writeFileAtomic (6+ temp+rename copies; credential-store's hardened variant stays independent by design); collapse session-store.ts's near-verbatim double message-parse loop (~L787–902); low: reuse isSafeSessionId/assertSafeSessionId from session-store in the other stores.

packages/ui + apps/desktop

  • chat-turn.tsx clipboard feedback — TurnFooterActions hand-rolls the copy-feedback state machine its same-file sibling already gets from useClipboardCopyFeedback; converge.
  • bot-incoming-main.ts — the new-bot-conversation sequence (limit check + rate-limit + getReadyConnection + createSession + set + emit + ensureSend) is copied twice (~L192–219, 266–300); call createBotConversationSession from both.
  • capability-snapshot.ts — the macOS TCC/OS permission-probing subsystem (~L347–459) is an independently evolving concern; split into permission-snapshot.ts.

Mechanical cleanup batches (low, one PR per area)

  • runtime + headless: git-exec wrapper (4 copies, drifted); harbor-cli's inline autonomous decision ladder → reuse the exported default policy; sleep copies in bots.
  • ui + cli + desktop: extract DeepResearchProgressPanel from chat-view.tsx (and re-point the desktop source-regex test at behavior); hoist the motion-collapse predicate that readStreamSnap/useStreamSnap duplicate from scroll-motion-policy.ts; merge smooth-stream.ts's resolveLiveBacklogCps/resolveCompletionCps; collapse CLI runtime-bootstrap.ts's 4 verbatim subscription-cloak modelFetch blocks into one helper; move main-window.ts's embedded smoke-diagnostic (~L502–585) to its own file; dedupe office-document-tool.ts's path-validation preamble.

Invariants

Same as #1084: behavior- and visually-neutral; preserve public imports (re-export compatibility where a moved symbol is public); no reverse imports from new leaf files; co-migrate tests; flat PRs off main; proportional verification (typecheck/build + targeted tests for pure moves). Per #1403, extraction PRs should pin their result with typed interface assertions and behavior tests, not new source-regex contracts.

Parallelization

All slices touch disjoint files except the compaction-helper batch (coordinate with #1374) and the OAuth lane (one owner, 2–3 ordered PRs). Before starting a slice, post a short claim comment here to avoid conflicts.

Out of scope

References

Activity

  1. chinawch007 commented on Jul 23, 2026

    @chinawch007
    Contributor

    Hi, I’d like to take the "Sandbox-denial predicate" slice.

    My plan is: extract one shared isLikelySandboxDenial({ stdout, stderr, sandboxed }) primitive (plus the regex it shares) into a new packages/runtime/src/sandbox/detect.ts, re-exported from sandbox/index.ts. The three copies differ only in input shape: two are semantically identical (same regex, same text shape, same boolean result — the 'sandboxed' in result guard in shell-tools.ts is just TS narrowing because BoundedShellResult lacks the field), and the shell-run-tool-result.ts variant additionally handles a PTY branch and checks sandboxExecution.enforced separately via its sandboxDenialForRecord wrapper. I'll lift the text-flattening (pipes-vs-pty) into the wrapper and pass a { stdout, stderr, sandboxed } triple into the new primitive, preserving the original behavior.

    Would it be okay for me to work on this?

  2. chinawch007 commented on Jul 24, 2026

    @chinawch007
    Contributor

    Implemented the sandbox-denial predicate slice in PR #1438 : deduped the three isLikelySandboxDenial copies into one {stdout, stderr, sandboxed} primitive in sandbox/detect.ts; each call site keeps its own input shaping (including the pty flattening in the variant), regex and behavior unchanged.

    Also a heads-up: this is my first contribution to the repo, so the CI workflows on the PR are awaiting maintainer approval to run. Thanks!

  3. me2seeks commented on Aug 3, 2026

    @me2seeks
    Contributor

    I'd like to take the Cron unification slice.

    I re-audited current main@6a4b4344: packages/core/src/plan-reminders.ts and packages/runtime/src/automation-state.ts still own independent five-field cron parsers, and there is no open PR touching either file.

    My implementation boundary will be:

    • establish one cron-expression module in @maka/core, with parsing, matching, and next-occurrence calculation consuming the same compiled representation;
    • migrate Plan Reminder and Automation to that module while preserving their existing public result/error contracts;
    • explicitly pin the current dialect differences instead of accidentally broadening Plan Reminder input while moving the code;
    • cover lists/ranges/steps, named month/day tokens, Sunday 0/7, Vixie day-of-month/day-of-week OR semantics, impossible dates, local-time DST behavior, malformed input, and the bounded search limit;
    • keep the change behavior-neutral outside the already-identified divergent parser behavior, with a flat PR from current main and no Runtime Host or scheduler ownership changes.

    I'll keep the commits reviewable: shared Core contract and tests first, followed by the two consumer migrations.

  4. chinawch007 commented on Aug 3, 2026

    @chinawch007
    Contributor

    I'd like to take the canonicalJson slice. Plan:
    Promote ab-manifest.ts's copy as the single source — export function canonicalJson, unchanged. Replace the 4 identical copies + divergent one with imports. Collapse hand-rolled sha256:...update(canonicalJson(...)) expressions into buildRunManifestFingerprint.
    Update the 3 test fixtures to import the authoritative copy.
    harness-oracle-registry.ts:534 is missing the .filter(([, v]) => v !== undefined), producing invalid JSON on {b: undefined}. Traced all 6 call sites — none can receive an undefined field (literals / JSON.parse / null-not-undefined), so the fix is behavior-neutral today and removes a latent footgun.

  5. chinawch007 commented on Aug 3, 2026

    @chinawch007
    Contributor

    Implemented task in #2005.
    Consolidated all six canonicalJson copies in @maka/headless into a single exported implementation in ab-manifest.ts, next to buildRunManifestFingerprint. Fixed the divergent copy in harness-oracle-registry.ts (missing undefined filtering — verified behavior-neutral across all call sites) and collapsed the hand-rolled sha256:...update(canonicalJson(...)) expressions into buildRunManifestFingerprint. Added behavior tests pinning the determinism invariants.

  6. chinawch007 commented on Aug 4, 2026

    @chinawch007
    Contributor

    Forgot claiming and just completed the task...😓

    Implemented the harbor-cli.ts slice in #2102 . harbor-cli.ts was re-deriving both the per-platform workspace path and the llm-connections.json schema/parse, and both copies had drifted from their authorities.

    Two changes:

    Moved the workspace-path authority (resolveMakaWorkspaceRoot) into @maka/storage, next to createConnectionStore, and made resolveDefaultConnectionsPath delegate to it (also fixes a latent win32 path-separator bug).

    Routed applyConnectionDefaults through ConnectionStore (getDefault/get) instead of its inline JSON.parse + hand-written schema, dropping the redundant normalizeProviderType patch the old code needed to paper over the drift.

  7. 8 remaining items

  8. chinawch007 commented on Aug 16, 2026

    @chinawch007
    Contributor

    Implemented slice Bot gateway bridge base in #3076

    Extracted a shared WsBridgeBase (connect, close policy, reconnect backoff, stop — including the shared reconnectBackoffMs) with a GatewayBridgeBase on top (opcodes, heartbeat/seq/session, identify/resume); QQ and Discord now override only the auth scheme, identify/resume payloads, dispatch event names, and send routes. DingTalk's Stream protocol has no opcode/heartbeat machinery, so it extends the WS base directly instead of the gateway layer. Follow-up done too: simple-bridge.ts's dead Feishu/Discord branches are removed and the file is renamed telegram-bridge.ts.

  9. added theissue type on Aug 29, 2026
  10. 99Gaoxiaoqi commented on Sep 6, 2026

    @99Gaoxiaoqi
    Contributor

    I am taking only the chat-turn.tsx clipboard-feedback slice of #1404. The change reuses useClipboardCopyFeedback in TurnFooterActions, preserving raw-text copying, pending-write deduplication, success/failure feedback, the 1400 ms reset, and unmount cleanup. It does not claim the other slices in this tracking issue.

    Automated message by OpenAI Codex on behalf of @99Gaoxiaoqi.

  11. seekskyworld commented on Sep 6, 2026

    @seekskyworld
    Contributor

    认领 Rive CLI 脱敏切片,具体范围与验收已拆到 #4925;只修改 rive-cli.ts / 相关测试,不占用其他成员的文件范围。

  12. seekskyworld commented on Sep 6, 2026

    @seekskyworld
    Contributor

    另认领存储 safe-ID 一致性切片,已拆为新 issue #4926;避开 agent-run-store、message-admission-store 及其现有 PR。

  13. HuYellow commented on Sep 15, 2026

    @HuYellow
    Contributor

    I am taking only the remaining compaction-helper deduplication slice: reuse the existing context-budget SHA-256 helper in ai-sdk-compaction and history-compact-checkpoint, and share the identical non-empty-string guard used by checkpoint validation and the Codex history compactor. I verified these copies still exist on main at dd15b63; #1374 is merged. Serializer, escaping, and token-estimation policies with different edge semantics will stay unchanged. This does not claim the other #1404 slices.

    Automated message by OpenAI Codex on behalf of @HuYellow.

  14. added
    staleNo qualifying activity within the lifecycle policy window
    on Oct 16, 2026
  15. github-actions commented on Oct 16, 2026

    @github-actions

    This issue has had no human activity for 30 days and has been marked stale. It will be closed in 30 days unless someone comments.

    If the issue is still current, please confirm it against the latest main and add any information that would help move it forward. Assigned issues and issues labelled pinned are exempt from this policy.

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

    staleNo qualifying activity within the lifecycle policy windowtrackingTracking or umbrella issue

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions