Phase 1: make a green make verify mean something - #276
Merged
Conversation
- Add rust-toolchain.toml at repo root pinning rustc 1.98.0 (current stable per D-11), with clippy+rustfmt components - Add Makefile fmt-check target (cargo fmt --check, no ICON_PATH prereq) - Add fmt-check to the verify prerequisite list
The dispatch-isolation sentinel is written per run to tell the isolation guard hooks which mode the phase resolved to. It is machine-local and must not be committed.
- Re-measured on the pinned rustc 1.98.0 toolchain: 75 violations, matching RESEARCH.md's count measured on 1.96.0 exactly - cargo clippy --fix --allow-dirty --allow-staged cleared 36 of them (needless_borrow, map_or->is_some_and, derivable_impls Default, io::Error::other, sort_by_key, last->next_back, eta-reductions) - cargo fmt normalizes the two files where --fix left non-idiomatic formatting (kordoc_lite.rs match guards, export/dispatch.rs Err arm) - cargo test --lib: 1199 passed, 0 failed, 3 ignored - no test fn touched by the diff, behavior preserved - 39 violations remain for Task 2's hand fixes
Hand-fixed the violation categories cargo clippy --fix could not touch automatically: - Genuinely dead test-only helpers in maru_dir.rs marked #[cfg(test)] rather than deleted - Two is_none_or usages (agents.rs, kakao_relay.rs, pre-dating this plan) rewritten to map_or(true, ...) - is_none_or needs rustc 1.82, the crate's declared MSRV (Cargo.toml, untouched by this plan) is 1.77.2 - manual_clamp, manual_strip, drop_non_drop, field_reassign_with_default, explicit_counter_loop, doc_lazy_continuation, filter().next_back(): one mechanical rewrite each, no behavior change - large_enum_variant / result_large_err: boxed the oversized variant/Err payload (inbox.rs, terminal/mod.rs, web_actions.rs); construction and match sites updated, Debug/Serialize output unchanged - too_many_arguments (9 functions): bundled related parameters into a small local struct per function. Two are #[tauri::command] IPC boundaries (skills_dispatch_background, terminal_spawn) - their paired frontend invoke() call was updated in the same commit to nest the same fields under one new key, so the wire values and all other callers are unchanged cargo clippy --offline -- -D warnings: 0 errors (was 75 at the start of this plan). cargo test --lib --offline: 1199 passed, 0 failed, 3 ignored - identical to the pre-fix baseline. cargo fmt --check: clean. pnpm typecheck: clean. Zero allow(clippy::...) or crate-level allow/deny/warn attributes introduced.
Add a clippy target (cargo clippy -- -D warnings, lib scope, $(ICON_PATH) prerequisite since it compiles the crate like test-rust) and add it to verify's prerequisite list after fmt-check. Proven red-then-green with a deliberate needless_return break in src-tauri/src/maru_dir.rs, reverted with git checkout -- with no residue (working tree byte-identical to before the break). Completes GATE-01 together with plan 01-01's fmt-check: make verify now fails on either a Rust format violation or a clippy warning.
Switch use.trace from on-first-retry to retain-on-failure so a first-attempt e2e failure in CI writes trace.zip under test-results/, which CI already uploads on failure. No retries key added (D-12): the suite keeps its zero-retry, no-flake signal from v0.4.58.
…dger The entry's premise (README naming inbox-processor/lint/hwpx-fill instead of the current inbox-process/vault-lint/hwpx skill names) no longer holds; the shipped ledger should not carry a stale claim into every e2e artifact. Add a one-line doc comment above the TODO_LEDGER declaration stating it is hand-maintained, not derived from README or REQUIREMENTS, following the sites.ts:267 comment convention.
…tion Proves success criterion 3 empirically per D-13: a failing e2e in CI must leave a downloadable Playwright trace in the uploaded artifacts, and must do so on the first failure with no retry (D-12 adds `trace: "retain-on-failure"` and deliberately does NOT add `retries`). This commit is reverted immediately after the CI run is observed. It is not intended to survive on the branch.
The clippy gate added in 01-02 passes on macOS but failed on CI (ubuntu-22.04) with 9 dead_code errors: browser_passkeys.rs and site_view.rs hold macOS-only functionality whose call sites compile out on Linux, leaving the helpers unreferenced, and -D warnings promotes dead_code to an error. Gate each item with #[cfg(target_os = "macos")], matching the convention both files already use. No allow escapes (D-08). Found by CI run 32558565444, the first real run of the new gate.
…2e assertion" This reverts commit e711b59.
- Add @types/node@^22.20.1 as a devDependency (approved at the blocking-human legitimacy gate), required for tsconfig.e2e.json/tsconfig.scripts.json to resolve types: ["node"] - Remove @types/dompurify from dependencies; dompurify@3.4.1 ships its own type declarations (GATE-06) - Fix: retype ScratchpadPane.tsx's two window.setTimeout-backed refs as number instead of ReturnType<typeof window.setTimeout>. Once @types/node's ambient globals become visible to tsconfig.app.json (which has no explicit types restriction), that type query resolves to NodeJS.Timeout instead of DOM's number, breaking these two browser-only timer refs. Type-only fix, no behavior change.
…te off
Add tsconfig.e2e.json at the repo root (strict, ES2022 + DOM + DOM.Iterable,
types: ["node"], include: ["e2e"]) and drive `pnpm exec tsc -p
tsconfig.e2e.json` to zero errors. Not yet added to tsconfig.json's
references array, so make verify stays green while this lands.
Re-measured against RESEARCH.md's "6 errors in 2 files" claim rather than
trusting it: confirmed exactly 6, in exactly those 2 files.
- e2e/drafts.spec.ts: give the in-page `drafts` seed array an explicit
DraftEntry type (importance/confidence/promotedTo all nullable) instead of
letting it narrow from the two seed literals, which didn't cover the
nullable values drafts_promote/drafts_create assign later
- e2e/helpers/todayFixtures.ts: merge a duplicate `taskId` field in
applyMutation's inline parameter type (TS2300/TS2717); narrow `event` to
{ ts?: string } before property access in read_task_events, matching the
existing `(row as {...})` narrowing convention used a few lines above for
taskRows
Deviation from the plan's literal instruction: left `composite: true` off
tsconfig.e2e.json. Adding it surfaced TS6307 project-boundary errors because
e2e/workbench-layout.spec.ts imports DEFAULT_MARU_SETTINGS from
src/lib/settings.ts, which composite's stricter file-list enforcement
rejects across the e2e/app project boundary. Verified empirically that
`tsc -b` accepts tsconfig.e2e.json as a solution-file reference without
composite (the plan's own note flagged this as untested), and every actual
acceptance criterion in Task 3/4 is satisfied without it. Fixing the
boundary "properly" would mean adding composite+declaration emission to
tsconfig.app.json, which is out of this plan's scope.
…ATE-03
Add { "path": "./tsconfig.e2e.json" } to tsconfig.json's references array.
typecheck is already a verify prerequisite (Makefile:309) and pnpm typecheck
is tsc -b, so this one line is GATE-03's e2e half; no Makefile change needed.
Break-and-revert proof (D-13): added `const gate03Probe: number = "not a
number";` to e2e/smoke.spec.ts, confirmed
`e2e/smoke.spec.ts(4,7): error TS2322: Type 'string' is not assignable to
type 'number'.` with tsc -b exiting 2, then reverted and confirmed
`pnpm typecheck` exits 0 with e2e/ clean.
…ipts/ errors - allowJs + checkJs true, strict false per D-10, lib ES2022+DOM for perf-startup-profile.mjs's page.evaluate/window callbacks - own tsBuildInfoFile, not yet referenced from tsconfig.json - re-measured against RESEARCH.md's 44/9: found 42 errors across 8 files
- releaseVersion.mjs, provisioningProfile.mjs, updaterManifest.mjs, publish-updater-manifest.mjs: add JSDoc @PARAM types for destructured options objects that checkJs could not infer past their default-bearing properties (42 of 42 errors traced to this one shape) - perf-startup-profile.mjs: inline @type cast on window inside Playwright page.evaluate/waitForFunction callbacks, matching e2e/startup.spec.ts's existing (window as Window & {...}) pattern for the same global - updaterManifest.test.mjs: tag signatureEntries()'s return as Array<[string, string]> so new Map(...) resolves to a tuple overload; drop a dead first 'release' key always shadowed by the later spread (TS1117), no behavior change - all fixes type-only: pnpm test 1853/1853 unchanged, lint:i18n/ check:select-chrome/icons:check unchanged output
…ip GATE-03
- tsconfig.json references gains { path: ./tsconfig.scripts.json }; 4 leaf
projects live under tsc -b now (app, node, e2e, scripts)
- no Makefile edit: typecheck was already a verify prerequisite
- break-and-revert proof: a deliberate number-for-string tag in
scripts/check-release-version.mjs failed tsc -b with TS2322, naming the
file; reverted, tsc -b green again, git status clean
- make verify run end to end: typecheck, lint gates, vitest 1853/1853,
cargo test 1199/1199, fmt-check, clippy -D warnings, and the frontend
build/bundle-budget check all green with all four project references live
- eslint@10.9.0, typescript-eslint@8.67.0, eslint-plugin-react-hooks@7.1.1
added as devDependencies (approved at the blocking-human legitimacy gate)
- eslint.config.js: ESM flat config, src/**/*.{ts,tsx} scoped to
tsconfig.app.json and e2e/**/*.ts scoped to tsconfig.e2e.json; exactly
react-hooks/rules-of-hooks, react-hooks/exhaustive-deps,
@typescript-eslint/no-unused-vars (with ^_ ignore patterns),
@typescript-eslint/no-floating-promises, no recommended preset extended,
no-console left off (D-07)
- package.json scripts.lint: eslint src e2e --max-warnings 0
- pnpm exec eslint src e2e measured 74 errors + 8 warnings across src/
(e2e/ clean), matching RESEARCH.md's prior measurement; not wired into
make verify yet (01-07's job)
- 12 no-unused-vars fixes: 6 dead icon-import removals, 1 dead prop-import removal, 2 dead useState bindings (outlookDecisions/setOutlookDecisions, telegramDecisions/setTelegramDecisions), 1 renamed to _gmailDecisions (setter still live), 1 dead useMemo (activeWorkspaceWriteReason), and the entirely-dead openBinaryWorkspaceFile callback plus its now-orphaned imports (binaryViewerClassify, binaryViewerPrepareAsset, insertBinaryTab, usesAssetProtocol, tabIdForWorkspaceFile) — deleting that dead callback also removed its own exhaustive-deps violation, so the final split is 12 no-unused-vars + 8 exhaustive-deps, not the inventoried 13+9 - 8 new eslint-disable-next-line react-hooks/exhaustive-deps comments, each naming the rule and carrying a same-line reason; no dependency array's contents were changed (confirmed via diff read), only comments added/edited or dead code removed - stale directive at (pre-edit) App.tsx:6974 removed; the two remaining pre-existing directives (boot-once-on-mount, kg-focus-reset) kept live and given same-line reasons to satisfy the same acceptance criterion - pnpm exec eslint src/App.tsx --max-warnings 0 exits 0; pnpm typecheck exits 0; pnpm test 1853/1853 unchanged
- 24 no-unused-vars: dead imports/bindings deleted, positional args (favoriteIds, settings, warnings, hdbg, headerBg) renamed with a leading underscore where the call site still needs the arity - 27 exhaustive-deps: every pre-existing violation annotated with a named, reasoned eslint-disable-next-line; no dependency array's contents changed - 1 no-floating-promises: void-marked the un-awaited Promise.all in WritingGuidelineSidebar - 7 dead no-console eslint-disable directives deleted (no-console stays off per D-07); all 35 console. calls in non-test src/ untouched pnpm exec eslint src --max-warnings 0 exits 0; pnpm typecheck exits 0; pnpm test 1853/1853 unchanged.
Add `make lint` (node_modules prerequisite, runs `pnpm lint`) and add it to the `verify` prerequisite list immediately after `typecheck`, per D-04. Rewrites the `verify` `##` gloss to cover all three gates this phase adds to it (lint, clippy, fmt-check). Deliberate-break proof (see SUMMARY for full output): - Wrong hook dependency list -> make lint fails naming react-hooks/exhaustive-deps; reverted, green again. - Unused symbol without a leading underscore -> make lint fails naming no-unused-vars; reverted, green again. make lint exits 0 on the reverted tree. Full make verify could not be run to a clean exit on this shared checkout (see SUMMARY); CI is the authoritative composite check per the team lead.
`retain-on-failure` records a trace for every test and discards the passes, so unlike the previous `on-first-retry` (which recorded nothing at all when `retries` is 0) it imposes real per-test cost. Measured against three pre-phase CI runs, the e2e suite went from a consistent 203 passed in 5.3-5.6m to 201 passed / 2 failed in 7.4m — roughly 33% slower, and enough to tip `select-audit.spec.ts` and `today.spec.ts:449` (both wall-clock `.poll()` waits) into flaking. DOM snapshots and screenshots are the bulk of that cost, so both are off. The network log and stacks are retained, which is what makes a CI failure diagnosable — the GATE-04 proof run downloaded a trace whose 471 KB `0-trace.network` and `0-trace.stacks` carried the useful signal. D-12 is unchanged: no `retries` key is added. The point of choosing `retain-on-failure` was to keep the suite's no-retry property, and buying trace capture at the cost of manufacturing flake would have defeated that.
The trace config was narrowed in a064994 (snapshots and screenshots off) after the only empirical trace.zip proof was captured, and no e2e failure has run since. Playwright documents retain-on-failure as writing trace.zip independent of those sub-flags, but 'documented to work' is a weaker bar than the D-13 empirical standard this phase held everywhere else. Reverted immediately after the CI artifact is inspected.
This reverts commit 986a8ce.
The comment added in a064994 said the narrowed trace retains the network log. That is wrong. Playwright's `snapshots: false` disables network capture along with DOM snapshots, so `0-trace.network` is 0 bytes. Measured from a CI probe against the narrowed config (run 32569215249): the trace.zip is 123,399 bytes across 6 entries — action timeline, failing stack, and source — versus 1,752,382 bytes across 14 entries with a 471 KB network log under the full configuration. GATE-04 itself still holds: a failing e2e in CI does produce a downloadable, non-empty trace.zip, re-proven under the config that actually ships. The correction is to what the trace contains, not to whether it exists. The comment now also names the failure class this trade-off is weakest against, since Phases 4-5 are the ones most likely to hit it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b79945cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The `verify` target grew four gates this phase but README still described the old sequence, so the project's documented contract no longer matched what CI enforces. Raised by Codex review on #276. - The command list gains `pnpm lint`, `make fmt-check`, and `make clippy`. - Both descriptions of what `make verify` runs now name ESLint and the Rust fmt/clippy gates. - A new paragraph records the three things a reader cannot infer from the target list: typecheck now spans four TypeScript projects including `e2e/` and `scripts/`, `rust-toolchain.toml` makes the Rust gates resolve the same compiler everywhere, and the CI trace artifact keeps the action timeline and stack but not DOM snapshots, screenshots, or the network log. Staged from HEAD plus these edits only: a concurrent session has an unrelated uncommitted README section in the working tree, which is deliberately left out of this commit and left intact on disk.
The previous fix (489aa6b) gated 4 test functions behind #[cfg(target_os = "macos")] because they called helpers that had to be gated to fix Linux dead_code errors. But cargo test --lib is a Linux-only CI step, so gating the tests to macOS meant they ran on no CI run at all -- silently dropped from the safety net despite being pure logic (integer-to-enum mapping, URL scheme filtering) with no macOS API dependency. Widen the gate on the helpers themselves from #[cfg(target_os = "macos")] to #[cfg(any(target_os = "macos", test))], so they compile whenever cfg(test) is set (any platform's `cargo test`) or on macOS proper, then drop the macOS-only gate from the four tests. `cargo clippy` never enables cfg(test), so the Linux dead_code fix still holds; `cargo test --lib` does, so the tests run again on Linux CI. Left EVENT_OPEN_REQUESTED and queue_opened_urls untouched -- no test references them, so widening would be unjustified. Found via Codex review on PR #276.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
make verifygains four gates it did not have: a Rust lint gate, a React hook-dependencygate, typecheck coverage for
e2e/andscripts/, and a pinned Rust toolchain. Plus aPlaywright trace that actually gets written on a CI failure, and two pieces of cleanup the
gates made possible.
Nothing user-visible changes. That is the point. Milestone 1's remaining phases are all
behavior-preserving refactors — collapsing duplicated Rust invariants, typing the IPC error
contract, moving
src/App.tsxpane state onto module stores — and a green gate is their onlyproof. This phase is first because the proof has to be trustworthy before the work starts.
The gates
cargo clippy -- -D warnings(lib scope), 75 pre-existing violations fixed, zeroallowescapescargo fmt --checkrust-version = "1.77.2"floorrust-toolchain.tomlpinning 1.98.0src/+e2e/["src"]src, node,e2e/,scripts/Every gate proven, not asserted
The phase held itself to one standard: break it, watch it go red, revert. Config inspection
was not accepted as proof for anything.
TS2322ine2e/smoke.spec.tsand a deliberateTS2304inscripts/build-macos-passkeys.mjseach drovepnpm typecheckto exit 2.Before this, a type error in a Playwright spec survived until the spec ran.
playwright-reportartifact downloaded, and
trace.zipconfirmed present and non-trivial. Then revertedbyte-identical. Done twice — once for the original config, again after the trace was
narrowed, because the second change invalidated the first proof.
make lintby name.The gate earned its keep on day one
make clippypassed on macOS and failed on CI with ninedead_codeerrors.browser_passkeys.rsandsite_view.rshold macOS-only functionality whose call sitescompile out on
ubuntu-22.04, orphaning the helpers. Fixed with#[cfg(target_os = "macos")]at the natural boundaries — sixteen attributes, no
allowsuppressions.The fix also had to reach four
#[cfg(test)]functions thatcargo clippynever compiles butcargo test --libdoes, which would have turned a clippy failure into a Linux test-compilefailure. That is a class of defect nobody in this repo could have seen before this phase.
What this cost, measured
retain-on-failurerecords a trace for every test and discards the passes, unlikeon-first-retrywithretries: 0, which records nothing. Turning it on took CI e2e from asteady 5.5m / 203 passed to 7.4m / 2 failed — the added time tipped two wall-clock
.poll()specs into flaking. Narrowing the trace to the action timeline and stacks restored 5.4m with
zero failures.
The remaining trace is 123 KB across 6 entries, against 1.75 MB across 14 with the full
config. It keeps the action timeline, the failing stack, and source. It does not keep DOM
snapshots, screenshots, or the network log —
snapshots: falsedisables network capture too,which the config comment now says explicitly after an earlier version of it got this wrong.
For a selector that stops matching or a re-render that drops content, a DOM snapshot is the
most useful diagnostic and it is not there. Phases 4-5 produce exactly that failure class, so
re-enabling
snapshotsis a two-minutes-of-CI decision they may want to make.What these gates still cannot catch
Recorded in
REQUIREMENTS.mdfor Phase 3 rather than left implicit.None of the seven gates can catch a serde mismatch at the Rust-TypeScript IPC boundary.
Verified concretely against this phase's own reshape of
SkillDispatchBackgroundArgs:cargo test --libconstructs the struct directly and never deserializes JSON, andmake test-e2eserves plainviterather thantauri-dev, sowindow.__TAURI_INTERNALS__never exists and neither reshaped command is exercised by any e2e spec. A camelCase drift
there passes typecheck, unit tests, e2e, and clippy, and fails only in the built app.
This is the pre-disclosed
native-tauri-e2e-runner-missinggap, deliberately kept open anddeferred. Phase 3 adds more boundary structs of that exact kind, so it inherits the blind spot
— with a suggested mitigation far cheaper than the deferred native runner: one
serde_json::from_valueround-trip test per new struct.Scope kept
no-consoleis not enabled; the 35 calls in non-testsrc/are untouched. The rule set isfour rules, not a preset.
scripts/is typechecked but not linted. The 45exhaustive-depsdisable comments added all carry a concrete reason and are meant to be deleted by Phases 4-5
as each pane is touched — they are a worklist, not a settlement.
Eight bare disable directives from July survive elsewhere in
src/, andgraph.spec.tshascarried its own per-spec retries since before this phase. Neither was introduced here, and the
phase artifacts now say so precisely rather than claiming a blanket property.
Verification
CI green at HEAD:
make verifypasses onubuntu-22.04with all seven gates wired in; Rust1192 passed; e2e 202 passed / 0 failed / 1 flaky (the pre-existing WebGL retry) in 5.5m.
Full goal-backward verification in
.planning/phases/01-trustworthy-verify-signal/01-VERIFICATION.md.🤖 Generated with Claude Code
https://claude.ai/code/session_0133xHs4682BJrwz8NHpK76p