Skip to content

refactor(settings): one appearance and state parser for every owner - #2932

Merged
thymikee merged 1 commit into
mainfrom
refactor/one-settings-parsing-owner
Sep 25, 2026
Merged

thymikee merged 1 commit into
mainfrom
refactor/one-settings-parsing-owner

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

parseAppearanceAction, parseSettingState, and summarizeCommandAttemptFailures existed
twice, in platform-apple/src/core/settings-parsing.ts and
platform-android/src/settings-parsing.ts. diff shows the two modules differed only by four
fallow-ignore-next-line code-duplication comments, and their test files differed only by the
import path of one assertion helper: the duplication was silenced rather than resolved, which is
exactly the "do not add guards that reconstruct another source of truth" trap.

One declaration now serves both owners:

  • appearance + state vocabularies → @agent-device/contracts/settings, beside
    parsePermissionAction / parseTextSizeCategory, which already share findVocabularyName
    and the same INVALID_ARGS refusal shape;
  • the command-attempt summary → @agent-device/kernel/errors, beside errorMessage.

A third copy existed in src/commands/capture/settings.ts, hand-listing
'light', 'dark', 'toggle' into its membership set. It now derives from the vocabulary, matching
how that file already builds setOf(...PERMISSION_ACTIONS). SETTINGS_APPEARANCE_USAGE is built
from the vocabulary too instead of restating it.

Acceptance is unchanged on every route. The state parser deliberately keeps lowercase-only
matching (no trim), so padding that used to be refused still is — pinned explicitly. 12 files,
net −8 production lines, and three duplicated sources of truth become one.

Validation

Tested at 55a2b905e.

  • pnpm check:affected --run — all runnable checks passed.
  • pnpm check:fallow --base origin/main — no issues in 13 changed files. Before the CLI
    derivation it flagged APPEARANCE_ACTIONS as an export with no consumer; deriving the CLI set
    from it resolved that at the owner instead of suppressing it. The four duplication
    suppressions are gone and no baseline entry referenced the deleted paths.
  • Byte-identity of the help surface is proven by pins that already existed: settings.test.ts
    asserts the literal 'appearance <light|dark|toggle>' in SETTINGS_FORMS and in the macOS
    refusal message, so deriving the usage string from the vocabulary cannot silently move a byte.
  • vitest --project unit-core over contracts/src/settings, kernel/src/errors,
    platform-apple …/app-settings, platform-android …/settings*, src/commands/capture,
    settings, cli-help — 25 files / 394 tests, plus the moved cases at their new owners.
  • oxlint . --deny-warnings, repo-wide oxfmt, tsc -b for the four packages, root tsc -p tsconfig.json.

No device lane is selected: no argv, wire shape, or route changes — the same values reach the
same tools.

`parseAppearanceAction`, `parseSettingState`, and `summarizeCommandAttemptFailures`
existed twice, in `platform-apple/src/core/settings-parsing.ts` and
`platform-android/src/settings-parsing.ts`. The two modules differed only by four
`fallow-ignore-next-line code-duplication` comments, and their test files differed
only by the import path of one assertion helper — the duplication was silenced
rather than resolved.

Both now consume one declaration:

- the appearance and state vocabularies move to `@agent-device/contracts/settings`,
  beside `parsePermissionAction` and `parseTextSizeCategory`, which already share
  `findVocabularyName` and the same refusal shape;
- the command-attempt summary moves to `@agent-device/kernel/errors`, beside
  `errorMessage`, which is what its payload travels with.

`SETTINGS_APPEARANCE_USAGE` is now built from the appearance vocabulary instead of
restating it, which the existing usage and macOS-refusal pins already prove is
byte-identical. A third hand copy in `src/commands/capture/settings.ts` derives its
membership set from the same declaration, matching how that file already builds the
permission sets, and a compile-time pin keeps the vocabulary equal to the published
client union without changing how that union is spelled in the shipped `.d.ts`.

Acceptance is unchanged on every route: the state parser keeps lowercase-only
matching, so padding that used to be refused is still refused.
@github-actions

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.81 MB 4.81 MB -319 B
Package (unpacked) 4.81 MB 4.81 MB -319 B
Package (download) 1.44 MB 1.44 MB -217 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.8 ms 29.3 ms -0.5 ms
CLI --help 85.6 ms 86.4 ms +0.8 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 55a2b90. This is a clean consolidation: settings parsing for appearance and state now goes through one owner instead of duplicated logic per surface.

All 18 checks are green. The changed paths are settings parsers and the error summarizer, which are pure functions with no device dependency, so unit coverage is the right check here, and the size report shows the bundle shrinking slightly, which matches a pure consolidation with no added surface.

I did not run the test suite locally. I compared the old and new parsing logic statically and found it byte-identical, so a live run would not add evidence for this refactor. The PR body says no argv, wire shape, or route changed, and tracing both call sites confirms that, so no device validation was needed for this change.

This is ready for human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 24, 2026
@thymikee
thymikee merged commit 05b3850 into main Sep 25, 2026
18 checks passed
@thymikee
thymikee deleted the refactor/one-settings-parsing-owner branch September 25, 2026 05:45
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-25 05:45 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.

1 participant