Skip to content

feat: add symbol-aware depgraph authority overlay - #2157

Merged
thymikee merged 3 commits into
mainfrom
codex/issue-2128
Aug 31, 2026
Merged

feat: add symbol-aware depgraph authority overlay#2157
thymikee merged 3 commits into
mainfrom
codex/issue-2128

Conversation

@thymikee

@thymikee thymikee commented Aug 31, 2026

Copy link
Copy Markdown
Member

Summary

Closes #2128

Adds a tooling-only, report-only, symbol-aware authority overlay to the static dependency graph.
It reuses the existing layering import parser and the central architecture-ownership declarations;
it preserves every existing JSON field and four-position edge tuple, and adds parallel
edgeAuthorities plus stable authorityCounts fields.

The overlay reports vocabulary, capability, live-state-shape, live-state-authority,
executable-policy, and ordinary. Import kind remains independent. Collapsed edges accumulate
all declared labels from their raw imports, so multi-label edges are retained. The README documents
that this is declared-authority evidence, not proof of behavioral ownership, safe removability, or
a score/threshold.

The authority classifier is a single declaration-derived rule table. Exact SessionState and
SessionStore roots and declared exports live in ARCHITECTURE_OWNERSHIP, with drift coverage;
the old depgraph constants are gone. The shared parser extracts named source symbols once and
handles comments inside bindings. The strict readNamedExports reader is used for capability and
live-state declaration checks; readDirectNamedExports remains for the R11 source scan, where its
intentional export *-tolerant semantics are still required. No second import parser was added.

Validation

  • Fresh implementation base: f152827447; merged prerequisite tooling: centralize machine-readable architecture ownership declarations #2126 / PR refactor(layering): centralize architecture ownership #2150 commit
    4244691e1ed0f71c148c412b8e645963f30a02b8 is an ancestor of final head 3e6fdb51ec.
  • Final origin/main: caa3dc23f9f3f6a829e91e8c5f00a1e4a3084f53. A three-way git merge-tree
    is clean; current main's replay/rank edits touch some layering files but introduce no genuine
    conflict or authority-overlay overlap requiring a rebase.
  • pnpm install --frozen-lockfile && pnpm build: passed on the fresh worktree.
  • pnpm depgraph:test: 24/24 passed, including all authority classifications, type-only
    SessionState, SessionStore authority/import kind, lookalike files, multi-label collapse, and
    legacy payload shape.
  • pnpm check:layering: 168/168 tests passed; layering guard OK.
  • Planted-red proof: adding a nonexistent MissingSessionStateExport to the shared declaration
    made pnpm check:layering fail at the live-state drift test (167 pass, 1 fail, exit 1).
    Restoring the declaration returned the focused depgraph suite to 24/24.
  • pnpm check:affected --run: exit 0; all runnable checks passed. The full 56-check set was
    selected; GitHub-authoritative native/device/coverage lanes were skipped locally.
  • Final diff adversarial re-audit: review findings on declaration centralization, generic
    classification, collapse alignment, ordinary projection, and shared comment-safe parsing are
    addressed; no remaining valid in-scope findings.

Exact report and jq evidence at final head:

pnpm depgraph --out /tmp/agent-device-2128-depgraph.json
jq '{generated, authorityCounts}' /tmp/agent-device-2128-depgraph.json
jq -r '. as $graph | range(0; ($graph.edges | length)) as $i | select($graph.edgeAuthorities[$i] != ["ordinary"]) | [($graph.edgeAuthorities[$i] | join("+")), $graph.nodes[$graph.edges[$i][0]].id, $graph.nodes[$graph.edges[$i][1]].id, ["value", "type", "dynamic"][$graph.edges[$i][2]]] | @tsv' /tmp/agent-device-2128-depgraph.json

Observed output: generated commit 3e6fdb51ec, 1,530 files, 7,923 edges; authority counts
vocabulary=155, capability=54, live-state-shape=114, live-state-authority=82,
executable-policy=80, ordinary=7441; the non-ordinary query returned 482 rows and the
multi-label query returned 3 collapsed edges.

Size

  • Touched files: 10, all under scripts/ or scripts/depgraph/README.md.
  • Gross diff: +459 / -15.
  • Move-adjusted net tooling LOC: +444; there are no file moves.
  • Production application LOC: 0; no src/ or packages/ files changed.
  • Unavoidable net tooling growth: depgraph overlay/payload +115 net lines; shared declaration and
    parser +46; focused regression tests +247; documentation +36. These sum to +444; deletions
    remove the obsolete duplicate authority extraction and parser/object-literal duplication.

Residual risks

  • Labels are deliberately limited to exact declared roots and named exports. Namespace, default,
    side-effect, and dynamic imports do not provide named-symbol evidence and are not inferred into
    a capability/state label.
  • The overlay is static declared-authority evidence; it does not establish runtime ownership
    quality, behavioral equivalence, or removability.
  • Native/device/provider/full-coverage proof remains GitHub-authoritative.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.53 MB 2.53 MB 0 B
JS gzip 847.4 kB 847.4 kB 0 B
npm tarball 973.6 kB 973.6 kB 0 B
npm unpacked 3.36 MB 3.36 MB 0 B

npm unpacked components

Component Base Current Diff
JS / dist source 2.68 MB 2.68 MB 0 B
Apple runner source/project 581.1 kB 581.1 kB 0 B
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.6 kB 45.6 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 23.9 ms 23.9 ms -0.0 ms
CLI --help 63.3 ms 64.0 ms +0.7 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

No changed packed files.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head c813b8ba47275f30a51ce483dfa8c42a21cb60b8. The report shape, parser reuse, multi-label collapse, tests, and green exact-head CI are otherwise sound, but there is one blocking ownership finding.

P1 — the overlay duplicates live-state authority declarations. scripts/depgraph/model.ts hardcodes src/daemon/types.ts + SessionState and src/daemon/session-store.ts + SessionStore, while merged #2150's ARCHITECTURE_OWNERSHIP declares neither. #2128 requires consuming only the shared declarations; this creates a second hand-synchronized authority list. Move these exact live-state root/symbol/label entries into scripts/layering/architecture-ownership.ts, have the overlay iterate that declaration, and delete the depgraph constants. Add a regression proving the live-state labels derive from the shared declaration while similarly named undeclared targets remain ordinary, then rerun pnpm depgraph:test, pnpm check:layering, pnpm check:affected --run, and exact-head CI.

@thymikee

Copy link
Copy Markdown
Member Author

Deep code-quality review

Verified locally on c813b8ba47: pnpm depgraph:test 23/23, pnpm check:layering 166/166 + guard OK. No file crosses 1k lines (scripts/depgraph/model.ts 477, scripts/layering/model.ts 476). Behaviour is right, the payload extension is backward-compatible, and swapping the capability parity check to readNamedExports while keeping readDirectNamedExports for the R11 source scan is correctly reasoned.

That said, I don't think this should merge as-is. The overlay hand-rolls a classifier for information the declarations already carry, and it puts two ownership facts in the consumer instead of in the module #2126 created to hold them.


1. The label already exists in the data. The overlay re-derives it as six branches.

ARCHITECTURE_OWNERSHIP declarations already carry kind, and every value is exactly the label this PR hardcodes:

declaration group kind label the PR hand-writes
vocabulary 'vocabulary' vocabulary
capabilities 'capability' capability
executablePolicies 'executable-policy' executable-policy

The overlay ignores kind entirely and re-derives the label from which array it happened to read, via three near-identical predicates plus two inline ifs. All five do the same thing: match one side of the edge against declared roots, optionally requiring a named symbol. That's a missing model, not five different rules.

The code-judo move is to let the declarations be the classifier:

type AuthorityRule = {
  label: DeclaredAuthorityLabel;
  side: 'source' | 'target';
  roots: readonly string[];
  /** When present, the import must name one of these. */
  symbols?: readonly string[];
};

const AUTHORITY_RULES: readonly AuthorityRule[] = [
  ...ARCHITECTURE_OWNERSHIP.vocabulary.map((d) => ({ label: d.kind, side: 'target', roots: d.roots })),
  ...ARCHITECTURE_OWNERSHIP.capabilities.map((d) => ({ label: d.kind, side: 'target', roots: [d.root], symbols: d.exports })),
  ...ARCHITECTURE_OWNERSHIP.liveState.map((d) => ({ label: d.kind, side: 'target', roots: [d.root], symbols: d.exports })),
  ...ARCHITECTURE_OWNERSHIP.executablePolicies.map((d) => ({ label: d.kind, side: 'source', roots: d.roots })),
];

function declaredAuthorities(edge: ResolvedImportEdge): DeclaredAuthorityLabel[] {
  return AUTHORITY_RULES.filter(
    (rule) =>
      rule.roots.some((root) =>
        matchesDeclaredRoot(rule.side === 'source' ? edge.file : edge.target, root),
      ) && (!rule.symbols || edge.symbols.some((symbol) => rule.symbols!.includes(symbol))),
  ).map((rule) => rule.label);
}

That deletes hasDeclaredVocabularyRoot, hasDeclaredCapabilitySymbol, hasExecutablePolicyOwner, both SESSION_*_ROOT constants, both inline ifs, and the whole body of authorityLabelsForEdge. Adding a seventh authority then becomes a data row in architecture-ownership.ts with zero depgraph edits — which is the property the issue was actually asking for.

Two things fall out for free:

  • hasDeclaredCapabilitySymbol currently uses edge.target === root while the other two use matchesDeclaredRoot. One code path, one rule, inconsistency gone.
  • edge.symbols.some((symbol) => exports.some((name) => name === symbol)) is a workaround for exports being a const-literal tuple, so .includes(symbol) won't typecheck. Once the rule table types symbols as readonly string[], it's just .includes. Related: ARCHITECTURE_OWNERSHIP is a bare as const with no declared type for vocabulary / capabilities / executablePolicies — unlike LOGICAL_MODULE_POLICIES, which has satisfies readonly LogicalModulePolicy[]. Giving the three groups declared types would stop consumers reverse-engineering their shape.

Keep AUTHORITY_LABELS as the explicit ordered list (stable JSON key order and zeroed counts both need it) and add a three-line test that every declaration kind appears in it. That's cheaper than the six branches and can't drift silently.

2. SESSION_STATE_ROOT / SESSION_STORE_ROOT are ownership facts living in the wrong module, with no drift protection.

const SESSION_STATE_ROOT = 'src/daemon/types.ts';
const SESSION_STORE_ROOT = 'src/daemon/session-store.ts';

The issue says "consume only the exact declarations established by #2126", and #2126 landed architecture-ownership.ts specifically to centralise this. Two of the six labels don't consume a declaration at all — they're literals in a consumer. Concretely, this is now the fourth hardcoding of 'src/daemon/types.ts' in the tooling (check.ts:294, daemon-modularity.ts:151, session-state.ts's R7 model, and now here). When four places enumerate the same fact, the question is what should be enumerating it — and #2126 already answered that.

The cost isn't aesthetic. Every other declaration group gets drift protection from architecture-ownership.test.ts:

  • architecture ownership roots resolve to tracked owners — every root must be a tracked file.
  • capability roots enumerate current exports and have production consumers — exact readNamedExports parity, so a renamed export fails loudly.

The two live-state pairs get none of it. Rename SessionState, move it out of types.ts, or rename SessionStore, and the overlay silently reports live-state-shape=0 / live-state-authority=0 with every gate green. A report whose zero is indistinguishable from "nothing declared" is worse than no report.

The fix is a liveState group with the same shape as capabilities:

liveState: [
  { name: 'session-state-shape', kind: 'live-state-shape',
    root: 'src/daemon/types.ts', exports: ['SessionState'] },
  { name: 'session-state-authority', kind: 'live-state-authority',
    root: 'src/daemon/session-store.ts', exports: ['SessionStore'] },
],

Add it to the tracked-root loop, and assert the declared exports are a subset of readNamedExports(root) (subset, not deepEqualtypes.ts exports plenty else). Roughly four test lines buy the drift protection, and finding #1 then absorbs both labels into the generic pass.

3. classifyEdgeAuthorities re-derives the grouping collapseEdges just performed.

collapseEdges already owns "one edge per (from, to) pair" and already builds the map. classifyEdgeAuthorities then walks every raw edge a second time to rebuild the same grouping, into a Map<string, ResolvedImportEdge[]> that retains every raw edge, and encodes the pair key a second time — once in edgePair() at model.ts:123, once inline in collapseEdges at model.ts:184. Two independently-maintained encodings of the same identity, and two functions that both own pair-grouping.

It also creates a positional-alignment contract between GraphData.edges and GraphData.edgeAuthorities that nothing prevents breaking — only a length assertion in the test detects it, and a future sort or filter on graph.edges would slide the labels one edge over with the lengths still equal.

Union the labels inside collapseEdges and hang them on GraphEdge.authorities. The strongest-kind continue needs the label set accumulated first, which is a few lines because the set carries forward on the retained edge:

const authorities = existing?.authorities ?? new Set<DeclaredAuthorityLabel>();
for (const label of declaredAuthorities(edge)) authorities.add(label);
if (existing && strength[existing.kind] >= strength[kind]) continue;
byPair.set(key, { ...,  authorities });

edgePair, classifyEdgeAuthorities and rawByPair all delete; one pass instead of two. build.ts then projects graph.edges.map((e) => [...e.authorities]) at the same site it builds the tuple array, so the parallel-array requirement from the issue is still met and the alignment is correct by construction rather than by convention. Making the defect unrepresentable beats asserting it away.

4. The ordinary sentinel is injected, stripped, then re-injected.

authorityLabelsForEdge adds 'ordinary' when the set is empty. classifyEdgeAuthorities then filters 'ordinary' back out of every raw result and re-adds it if the union is empty. The canonical-ordering AUTHORITY_LABELS.filter(...) therefore runs twice per edge, once for a value that's thrown away.

Worse, it makes the exported function disagree with the report: authorityLabelsForEdge(rawEdge) can return ['ordinary'] for an edge whose collapsed pair is labelled live-state-shape. That's two definitions of the same word, and the export exists only for the test at model.test.ts:139.

Make the pure function declaredAuthorities(edge): DeclaredAuthorityLabel[] returning [] when nothing is declared, and apply the ordinary default exactly once at the projection boundary. AuthorityLabel = DeclaredAuthorityLabel | 'ordinary' also stops the type claiming "no authority" is a kind of authority.

5. importedSymbols duplicates the specifier parsing sitting directly above it — and silently drops symbols on legal input.

importedSymbols and statementIsTypeOnly are adjacent and both run /\{([\s\S]*?)\}/.exec(statement), both .split(','), both .trim(), both look for a leading type. Two hand-rolled specifier parsers that must agree about what a specifier is; if they drift, typeOnly and symbols describe different statements.

They already disagree with the language. Probed against the branch:

"import { SessionState /* live state */ } from './types.ts';"
  => symbols: ["SessionState /* live state */"]

"import {\n  SessionState,\n  // Other,\n  SessionRef,\n} from './types.ts';"
  => symbols: ["SessionState", "// Other", "SessionRef"]

Both are legal TypeScript. The first silently loses its live-state-shape label; the second emits a comment as a symbol name. Nothing in the repo hits it today, so this is latent rather than live — but it's silent by construction: report-only means no gate ever fails, so the label just quietly disappears.

Extract one parseNamedSpecifiers(statement) that both consume, and take the leading identifier rather than the raw text:

const name = /^[A-Za-z_$][\w$]*/.exec(specifier.trim().replace(/^type\s+/, ''))?.[0];

That handles a as b, a /* c */ and rejects // b in one expression. (Minor while you're in there: .split(/\s+as\s+/, 1)[0]?.trim() — the limit-1 split always yields a string, so the ?. is dead; plain .split(/\s+as\s+/)[0] reads better.)

Worth noting that facade-exports.ts, in the same directory, argues at length against exactly this: "a regex has to enumerate every export FORM by hand, and the one it forgets is exactly the one that slips through." I'm not asking you to move parseImports to oxc — that's out of scope and the layering gate is line-based on purpose — but the extracted helper should at least tokenise rather than string-match.


Not blocking

  • Reusing the layering parser instead of adding a second one: right call, matches the issue's stop condition.
  • The readDirectNamedExportsreadNamedExports swap: correct, and it makes the capability parity check strictly stricter. readDirectNamedExports is genuinely still needed at package-boundaries.test.ts:287.
  • README caveats (declared authority, not behavioural ownership / removability / a score) are the right framing and clearly written.
  • The namespace/default/side-effect/dynamic blind spot is documented in both the README and the PR body. Fine as a stated limit.
  • The fixture is good — same-pair multi-label collapse, type-only vs value on the same target, and a lookalike undeclared file are all exercised.

Verdict

Requesting changes on #1 and #2. #1 is the one that matters: the overlay currently hand-writes a classifier for a label the declaration data already carries, so every future authority costs a predicate and an if in depgraph/model.ts instead of a row in architecture-ownership.ts. #2 is the correctness consequence — two ownership facts outside the ownership module, with no drift test, in a report where a wrong answer looks identical to an empty one.

#3#5 are smaller but land in the same restructure. Taken together this should come out net shorter than the current +336, with fewer concepts to hold and one place to change.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed in beebc4955b.

  • Moved the exact SessionState and SessionStore root/symbol/label entries into
    ARCHITECTURE_OWNERSHIP; the overlay iterates that declaration and the duplicate depgraph
    constants are deleted.
  • Added a regression for shared-declaration derivation and ordinary same-named lookalikes.
  • Hardened the existing import parser's named-symbol extraction against comments, with a focused
    parser regression.

Validation is updated in the PR body: pnpm depgraph:test 24/24, pnpm check:layering 167/167
plus guard OK, planted-red live-state proof (23 pass / 1 fail before restoration), and
pnpm check:affected --run exit 0 with all runnable checks passing. The exact committed-head
report was regenerated at beebc4955b.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 31, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head beebc4955b81eb26681be22c09d494c5b346d6bb. The prior duplicate-authority finding is fixed: depgraph now consumes the declarations from ARCHITECTURE_OWNERSHIP, with declaration and similarly-named-target regression coverage. The import-comment normalization is focused parser hardening with tests. No code findings. Tooling-only change; no device evidence owed. Exact-head checks are green except iOS Smoke still in progress; code review is ready for human review pending that lane.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 31, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Addressed the deep review in final head 3e6fdb51ec.

Validation: pnpm depgraph:test 24/24; pnpm check:layering 168/168 plus guard; planted missing-export proof failed at the expected live-state drift test (167 pass / 1 fail), then the restored suite passed; pnpm check:affected --run exit 0 with all runnable checks passing. Final report: 1,530 files, 7,923 edges, 482 non-ordinary rows, 3 multi-label collapsed edges. Gross diff is +459/-15, move-adjusted net tooling +444, with 0 production LOC. The updated PR body contains the exact /tmp and jq commands and residual diagnostic-only risks.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 31, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 3e6fdb51ecc71cce423eb6d54262787b599fa8bb. No code findings. The prior duplicate-authority issue remains fixed: every overlay rule derives from ARCHITECTURE_OWNERSHIP, collapsed-edge union is now owned by collapseEdges, and parser normalization remains focused and covered. Code review is ready for human review. Android/iOS Smoke, Coverage, and Repo Guards are still in progress; refresh the PR body’s stale beebc4955b exact-head/local-validation claim and stamp pnpm depgraph:test, pnpm check:layering, and pnpm check:affected --run on this head.

@thymikee
thymikee merged commit a79f0d2 into main Aug 31, 2026
18 checks passed
@thymikee
thymikee deleted the codex/issue-2128 branch August 31, 2026 09:54
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-31 09:54 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tooling: add a symbol-aware authority overlay to depgraph

1 participant