diff --git a/docs/codebase-walkthrough.md b/docs/codebase-walkthrough.md index 009772d..bc04a00 100644 --- a/docs/codebase-walkthrough.md +++ b/docs/codebase-walkthrough.md @@ -24,7 +24,7 @@ A complete guide to understanding how FinalRun works, from CLI invocation to tes ## 1. Architecture Overview -FinalRun is a **monorepo with 5 packages** that together form an AI-powered mobile app testing tool: +FinalRun is a **monorepo with 7 packages** that together form an AI-powered mobile app testing tool: ```text finalrun-ts/ @@ -33,7 +33,9 @@ finalrun-ts/ │ ├── cli/ CLI commands, test orchestration, report server, artifact writing │ ├── goal-executor/ AI agent loop: screenshot → LLM plan → device action → repeat │ ├── device-node/ Device management: gRPC driver, screenshots, action execution -│ └── report-web/ Next.js web app for viewing reports (alternative to built-in server) +│ ├── cloud-core/ Cloud-submission logic, bundled into the compiled CLI binary +│ ├── local-runtime/ Per-platform runtime asset bundle (driver APKs/iOS zips, proto, report SPA dist) +│ └── report-web/ React SPA (Vite) for viewing reports, served by the CLI's report server ``` **High-level flow:** @@ -47,7 +49,7 @@ Report displayed Artifacts saved to disk AI executes test on device (HTML in browser) <────────────────────────── (screenshot → LLM → tap/type → repeat) ``` -### Why 5 packages? +### Why 7 packages? | Package | Reason for separation | |---------|----------------------| @@ -55,7 +57,9 @@ Report displayed Artifacts saved to disk AI executes test on device | `cli` | User-facing. Handles I/O, config, orchestration. Should not know about gRPC or LLM internals. | | `goal-executor` | The AI loop is complex enough to deserve isolation. It could theoretically be swapped for a different execution strategy. | | `device-node` | Platform-specific (Android/iOS) device code. Isolates gRPC, ADB, and driver concerns from business logic. | -| `report-web` | Optional Next.js frontend. The CLI has a built-in server too, so this is an enhancement, not a dependency. | +| `cloud-core` | Pure cloud-submission logic kept free of CLI I/O, so it can be bundled into the compiled CLI binary. | +| `local-runtime` | Builds the per-platform runtime asset tarball (driver APKs/iOS zips, gRPC proto, report SPA dist) the compiled CLI needs at runtime. | +| `report-web` | React SPA (Vite) frontend that the CLI's report server serves. Its Vite bundle is copied into `cli/dist/report-app` at build time (`copyReportApp.mjs`) and into the runtime tarball (`buildRuntimeTarball.mjs`) — both builds fail without it, so it is a build-time dependency of the CLI, not an optional alternative. | --- @@ -188,7 +192,7 @@ finalrun doctor Check host readiness finalrun test auth/login.yaml --env staging --platform android --model openai/gpt-5.4-mini ``` -**What happens in `runTestCommand()` (bin/finalrun.ts:271-350):** +**What happens in `runTestCommand()` (bin/finalrun.ts:372-485):** ```text 1. normalizeTestSelectors() Split comma-separated selectors, trim whitespace @@ -214,7 +218,7 @@ finalrun test auth/login.yaml --env staging --platform android --model openai/gp ## 4. Phase 1: Validation (`runCheck`) -**File:** `packages/cli/src/checkRunner.ts` +**File:** `packages/common/src/checkRunner.ts` Before touching any device, FinalRun validates everything: @@ -675,10 +679,10 @@ Then `rebuildRunIndex()` scans all run directories and regenerates `runs.json` FinalRun has two ways to display reports: -| | Built-in Server (`cli`) | Next.js App (`report-web`) | +| | Built-in Server (`cli`) | React SPA (`report-web`) | |---|---|---| | **File** | `packages/cli/src/reportServer.ts` | `packages/report-web/` | -| **Technology** | Raw `http.createServer()` | Next.js App Router | +| **Technology** | Raw `http.createServer()` | Vite + React SPA | | **When used** | Default (`finalrun start-server`) | Development or custom deploy | | **Routes** | Same | Same | @@ -1093,31 +1097,41 @@ Quick reference: where to find what. | `TestDefinition.ts` | `TestDefinition`, `BindingReference` | What a test looks like after YAML parsing | | `TestResult.ts` | `TestResult`, `AgentAction`, `FirstFailure`, `TestStatus` | What a test result looks like after execution | | `SuiteDefinition.ts` | `SuiteDefinition` | What a suite looks like after YAML parsing | -| `DeviceAction.ts` | `TapAction`, `EnterTextAction`, `ScrollAbsAction`, ... (18 types) | All possible device actions | +| `DeviceAction.ts` | `TapAction`, `EnterTextAction`, `ScrollAbsAction`, ... (22 concrete action classes) | All possible device actions | | `Environment.ts` | `AppConfig`, `EnvironmentConfig`, `RuntimeBindings` | Environment and binding types | | `Trace.ts` | `AgentActionTrace`, `TraceSpan`, `TimingInfo` | Performance tracing types | | `Hierarchy.ts` | `Hierarchy`, `HierarchyNode` | UI element tree from device | -### `packages/cli/src/` +### `packages/common/src/` (non-model modules) | File | What it does | Key functions | |------|-------------|---------------| -| `bin/finalrun.ts` | CLI entry point, command definitions | `runTestCommand()` | -| `testRunner.ts` | Main test orchestrator | `runTests()` | | `checkRunner.ts` | Validation phase | `runCheck()` | -| `sessionRunner.ts` | Device setup + test execution | `prepareTestSession()`, `executeTestOnSession()` | | `testLoader.ts` | YAML file parsing | `loadTest()`, `loadTestSuite()`, `loadEnvironmentConfig()` | | `testSelection.ts` | Test file discovery | `selectTestFiles()`, `expandSelector()` | -| `testCompiler.ts` | Test → AI prompt | `compileTestObjective()` | | `workspace.ts` | Workspace discovery | `resolveWorkspace()`, `loadWorkspaceConfig()` | | `appConfig.ts` | App configuration | `resolveAppConfig()` | -| `env.ts` | Environment variables | `CliEnv`, `parseModel()`, `resolveApiKey()` | +| `env.ts` | Environment variables | `CliEnv` | +| `constants.ts` | Shared constants, model string parsing | `parseModel()`, `parseReasoningLevel()` | + +### `packages/cli/bin/` + +| File | What it does | Key functions | +|------|-------------|---------------| +| `finalrun.ts` | CLI entry point, command definitions | `runTestCommand()` | + +### `packages/cli/src/` + +| File | What it does | Key functions | +|------|-------------|---------------| +| `testRunner.ts` | Main test orchestrator | `runTests()` | +| `sessionRunner.ts` | Device setup + test execution | `prepareTestSession()`, `executeTestOnSession()` | +| `testCompiler.ts` | Test → AI prompt | `compileTestObjective()` | +| `apiKey.ts` | API key resolution | `resolveApiKey()` | | `reportWriter.ts` | Artifact writing | `ReportWriter` class | | `runIndex.ts` | Run index management | `rebuildRunIndex()`, `loadRunIndex()` | | `reportServer.ts` | Built-in HTTP server | `serveReportWorkspace()` | | `reportServerManager.ts` | Server lifecycle | `startOrReuseWorkspaceReportServer()` | -| `reportTemplate.ts` | Run detail HTML | `renderHtmlReport()` | -| `reportIndexTemplate.ts` | Run index HTML | `renderRunIndexHtml()` | | `hostPreflight.ts` | SDK checks | `runHostPreflight()` | ### `packages/goal-executor/src/` diff --git a/docs/memory/common/env.md b/docs/memory/common/env.md index 6785ee6..5e22d8e 100644 --- a/docs/memory/common/env.md +++ b/docs/memory/common/env.md @@ -111,16 +111,19 @@ the env module that no consumer actually needs. carries a comment naming it a backward-compatibility shim. The block retains all nine symbols even though no production module imports through it. **Why**: `env.test.ts:6` imports `parseModel` and `parseReasoningLevel` from `../env.js`, and the -constitution's Test Integrity rule forbids editing a test to chase a moved import path — a test -conforms to the spec, and rewriting it to make a refactor compile inverts that. Keeping the block +re-export is what keeps that import path resolving — the shim exists for consumer compatibility with +the one line in the repo that still reaches these symbols through `env.js`. Keeping the block also makes `env.js`'s export surface independent of where the validators are defined, which is what lets a move like this one claim zero observable behavior change. The block's justification is exactly that one import line: no production module imports anything but `CliEnv` from `env.js`, and `MODEL_FORMAT_EXAMPLE`, `PROVIDER_ENV_VARS`, `SUPPORTED_AI_PROVIDERS`, `SUPPORTED_AI_PROVIDERS_LABEL`, `ParsedModel`, and `SupportedProvider` have no `env.js` consumer anywhere in the repo. -**Rejected**: (a) deleting the barrel and repointing importers — forces an import-path edit in -`env.test.ts`, which Test Integrity prohibits; (b) leaving a duplicate definition in `env.ts` so +**Rejected**: (a) deleting the barrel and repointing `env.test.ts`'s import at `../constants.js` — +an edit the constitution's Test Integrity rule permits (it allows updating a test to match the spec, +and prohibits only reshaping *implementation* to suit test infrastructure), but one that narrows +`env.js`'s export surface for no consumer's benefit, and that surface stability is what makes the +move's zero-observable-change claim checkable; (b) leaving a duplicate definition in `env.ts` so the package root's export set stays literally identical — two definitions of the same validator is exactly the drift a single home exists to prevent. *Introduced by*: 260731-65sg-env-structural-refactor-pilot diff --git a/docs/memory/common/hierarchy.md b/docs/memory/common/hierarchy.md index 43dee4f..933e2a2 100644 --- a/docs/memory/common/hierarchy.md +++ b/docs/memory/common/hierarchy.md @@ -1,6 +1,6 @@ --- type: memory -description: "UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, consumed by 16 files in goal-executor/device-node): `fromJsonString` dispatches array→flat / object→tree, and the paths are deliberately not equivalent — only the flat path shortens `:id/` ids, infers `isImage` from the class, takes `identifier`; alias resolution is `??`-presence via `_pick`/`orDefault`, never truthiness, so `false` and `''` survive; bounds are a 4-array or a left/top/right/bottom object; reads are unvalidated casts." +description: "UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, consumed by four goal-executor files): `fromJsonString` dispatches array→flat / object→tree, and the paths are deliberately not equivalent — only the flat path shortens `:id/` ids, infers `isImage` from the class, takes `identifier`; alias resolution is `??`-presence via `_pick`/`orDefault`, never truthiness, so `false` and `''` survive; bounds are a 4-array or a left/top/right/bottom object; reads are unvalidated casts." --- # UI Hierarchy Parsing (common) @@ -10,9 +10,16 @@ description: "UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, con `packages/common/src/models/Hierarchy.ts` turns the driver's UI-hierarchy JSON into `HierarchyNode`s and the flattened list that `toPromptElementsForPlanner` / `toPromptElementsForGrounder` build -planner and grounder prompts from. It is the most-depended-on parser in the repo — 16 files across -`goal-executor` and `device-node` consume `Hierarchy`/`HierarchyNode` — and its one production entry -point is `Hierarchy.fromJsonString`, called on the device-capture path in +planner and grounder prompts from. Every consumer of `Hierarchy`/`HierarchyNode` lives in +`goal-executor` — `ai/AIAgent.ts`, `ActionExecutor.ts`, `TestExecutor.ts` and +`GrounderResponseConverter.ts`, each importing through the `@finalrun/common` barrel. +**`device-node` consumes neither type**, even though it is the client that fetches every payload +(`GrpcDriverClient.getHierarchy` / `getScreenshotAndHierarchy`) and owns a +`DeviceScreenshotAndHierarchy` interface of its own: the hierarchy crosses that package as an opaque +JSON `string` — the declared field type in both `GrpcDriverClient`'s response and +`DeviceRuntime.DeviceScreenshotAndHierarchy` — and only `goal-executor` parses it. So a grep for +`Hierarchy` under `device-node` hits RPC names and that local interface, never this type. +The one production entry point is `Hierarchy.fromJsonString`, called on the device-capture path in `packages/goal-executor/src/TestExecutor.ts`. A behaviour change here surfaces as a grounding failure at runtime rather than as a test failure, so the parse contract is pinned by mutation-verified characterization tests in `packages/common/src/models/test/Hierarchy.test.ts` diff --git a/docs/memory/common/index.md b/docs/memory/common/index.md index efd7967..966b491 100644 --- a/docs/memory/common/index.md +++ b/docs/memory/common/index.md @@ -8,4 +8,4 @@ description: "`packages/common` — the base of the dependency graph, imported b | File | Description | |------|-------------| | [env](env.md) | `CliEnv` environment loading (`packages/common/src/env.ts`): `load` layers `.env.` → plain `.env` (fill-only) → OS env (highest precedence), `includeDotEnv` opts out only on a literal `false`, and `getRequired`'s falsy check makes an empty string as missing as an absent key. Model and reasoning-level validation live in `constants.ts` beside their level lists, while `env.ts`'s re-export block is a deliberate backward-compat shim for the one import path that still routes through it. | -| [hierarchy](hierarchy.md) | UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, consumed by 16 files in goal-executor/device-node): `fromJsonString` dispatches array→flat / object→tree, and the paths are deliberately not equivalent — only the flat path shortens `:id/` ids, infers `isImage` from the class, takes `identifier`; alias resolution is `??`-presence via `_pick`/`orDefault`, never truthiness, so `false` and `''` survive; bounds are a 4-array or a left/top/right/bottom object; reads are unvalidated casts. | +| [hierarchy](hierarchy.md) | UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, consumed by four goal-executor files): `fromJsonString` dispatches array→flat / object→tree, and the paths are deliberately not equivalent — only the flat path shortens `:id/` ids, infers `isImage` from the class, takes `identifier`; alias resolution is `??`-presence via `_pick`/`orDefault`, never truthiness, so `false` and `''` survive; bounds are a 4-array or a left/top/right/bottom object; reads are unvalidated casts. | diff --git a/docs/memory/device-node/log-capture.md b/docs/memory/device-node/log-capture.md index d292229..7d9ab04 100644 --- a/docs/memory/device-node/log-capture.md +++ b/docs/memory/device-node/log-capture.md @@ -164,8 +164,8 @@ and `finalizeQuietly`'s own `Logger.e` is guarded for the same reason: `finalize recorded error is what makes that `catch` reachable on the very failure the guard exists for, and `finalizeQuietly` MUST resolve for callers that are already returning a failure. The reason is `Logger.e`'s **independent** fallibility: `Logger._emit`'s sink -loop (`packages/common/src/logger.ts:103-105`) runs every sink with no `try`/`catch`, and the CLI -installs `ReportWriter.createLoggerSink()` (`packages/cli/src/reportWriter.ts:132`), a bare +loop (`packages/common/src/logger.ts`) runs every sink with no `try`/`catch`, and the CLI +installs `ReportWriter.createLoggerSink()` (`packages/cli/src/reportWriter.ts`), a bare synchronous `fs.appendFileSync`, so a full disk, a permissions change or a removed artifacts directory makes the log call throw on its own schedule. The two failures can also be one, but only conditionally: the device log lives at `/finalrun-logs/…` and the runner log at diff --git a/docs/memory/drivers/grpc-contract.md b/docs/memory/drivers/grpc-contract.md index e2fc400..f016a34 100644 --- a/docs/memory/drivers/grpc-contract.md +++ b/docs/memory/drivers/grpc-contract.md @@ -124,10 +124,19 @@ per tick, for a request that asked for 24 frames a second. Both drivers state th computation lives: Android's `TestUtils.calculateFrameDelay` uses `1000.0 / fps.toDouble()`, and the Swift `XCViewHierarchyManager` cites it. -`fps` MUST be clamped to `1...60` (`streamingFpsRange`), matching Android's `coerceIn(1, 60)`: below 1 -the interval is unbounded, above 60 the hierarchy snapshot cannot keep up. An omitted `fps` defaults -to `1` (`defaultStreamingFps`) — the proto's documented 24 is not adopted, because changing the -default is a separate behaviour change from fixing the arithmetic. +`fps` MUST be clamped to `1...60` (`streamingFpsRange`), matching Android's `coerceIn(1, 60)`. The +clamp guards the divisor: in `GrpcDriverServer.swift`'s integer `1_000_000_000 / fps`, an fps of `0` +is a division by zero and a negative fps a negative-to-`UInt64` conversion — both Swift traps that +kill the XCUITest runner — while the floating-point paths (`calculateFrameDelay`, +`XCViewHierarchyManager`) turn an fps below 1 into an unbounded or negative interval. The upper +bound of 60 is the chosen cap on hierarchy-snapshot load shared by all three paths, not a measured +ceiling. An omitted `fps` on the +gRPC `StartStreaming` path defaults to `24` on both drivers (`GrpcDriverServer.swift`'s +`defaultStreamingFps`, and the inline `if (request.hasFps()) request.fps else 24` in Android's +`DriverServiceImpl.startStreaming` — both adopting the proto's documented default); +`XCViewHierarchyManager`'s `defaultStreamingFps` of `1` defaults only the legacy WebSocket timer +path. The two are deliberately not aligned — aligning them would change the RPC's behaviour rather +than just guard its arithmetic. #### Scenario: streaming at 24 fps diff --git a/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/DriverServiceImpl.kt b/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/DriverServiceImpl.kt index 9e788ef..8c84c68 100644 --- a/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/DriverServiceImpl.kt +++ b/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/DriverServiceImpl.kt @@ -36,8 +36,8 @@ import kotlinx.coroutines.withTimeoutOrNull /** * gRPC service implementation for the Android driver. * - * This replaces ActionProcessor and handles all incoming RPC calls from the - * TypeScript client (packages/device-node). + * Handles all incoming RPC calls from the TypeScript client + * (packages/device-node). * Each method corresponds to an action that can be performed on the device. */ class DriverServiceImpl : DriverServiceGrpc.DriverServiceImplBase() { diff --git a/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/GrpcDriverServer.kt b/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/GrpcDriverServer.kt index 059a0be..381db12 100644 --- a/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/GrpcDriverServer.kt +++ b/drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/GrpcDriverServer.kt @@ -10,8 +10,8 @@ import java.util.concurrent.TimeUnit /** * gRPC server for the Android driver. * - * This replaces WebSocketServerImpl. It starts a gRPC server on the specified port - * and handles incoming RPC calls from the TypeScript client (packages/device-node). + * Starts a gRPC server on the specified port and handles incoming RPC calls + * from the TypeScript client (packages/device-node). */ class GrpcDriverServer(private val port: Int) { private var server: Server? = null diff --git a/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.history.jsonl b/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.history.jsonl index fd1bbcf..03d997c 100644 --- a/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.history.jsonl +++ b/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.history.jsonl @@ -9,3 +9,4 @@ {"event":"review","result":"passed","ts":"2026-07-31T02:29:55Z"} {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T02:33:34Z"} {"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-31T02:35:20Z"} +{"event":"review","result":"passed","ts":"2026-07-31T12:20:33Z"} diff --git a/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml b/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml index a7a28b7..416c3a9 100644 --- a/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml +++ b/fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml @@ -10,7 +10,7 @@ progress: review: done hydrate: done ship: done - review-pr: active + review-pr: done plan: generated: true task_count: 7 @@ -34,7 +34,7 @@ stage_metrics: review: {started_at: "2026-07-31T02:19:15Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T02:29:55Z"} hydrate: {started_at: "2026-07-31T02:29:55Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T02:33:34Z"} ship: {started_at: "2026-07-31T02:33:34Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T02:35:20Z"} - review-pr: {started_at: "2026-07-31T02:35:20Z", driver: git-pr, iterations: 1} + review-pr: {started_at: "2026-07-31T02:35:20Z", driver: git-pr, iterations: 1, completed_at: "2026-07-31T12:20:33Z"} prs: - https://github.com/droid-ash/finalrun-agent/pull/170 change_type_source: explicit @@ -50,4 +50,4 @@ true_impact: computed_at_stage: ship summary: 'Deleted 1,864 lines of dead code: TestActions.kt (whole file), XCTestManager.swift commented-out code blocks, 30 unreferenced constants.ts exports, and isInteractive/resolveCliCacheRoot in packages/cli' # true_impact: lazily created on first stage-finish that computes it (no placeholder here). -last_updated: 2026-07-31T02:35:20Z +last_updated: 2026-07-31T12:20:33Z diff --git a/fab/changes/260731-xkfl-correct-false-stale-claims/.history.jsonl b/fab/changes/260731-xkfl-correct-false-stale-claims/.history.jsonl new file mode 100644 index 0000000..4b6cdf2 --- /dev/null +++ b/fab/changes/260731-xkfl-correct-false-stale-claims/.history.jsonl @@ -0,0 +1,16 @@ +{"action":"enter","driver":"fab-new","event":"stage-transition","stage":"intake","ts":"2026-07-31T12:04:07Z"} +{"args":"Correct false and stale factual claims across comments, docs and policy (items from adversarial review of PRs #168-#175, re-verified at main 91b2683): code-quality.md drivers.yml false 67/18 ratio; code-review.md stale \"planned\" sweep (shipped as #171); env.ts false constitution citation; logger.ts citation rot in logWriteStream.ts + log-capture.md; false used-by headers in constants.ts, Hierarchy.ts, DriverServiceImpl.kt, GrpcDriverServer.kt; grpc-contract.md wrong fps default; codebase-walkthrough.md five false claims; 3vhw .status.yaml stuck at review-pr active. Docs/comments only, no runtime changes. Do not commit fab/backlog.md.","cmd":"fab-new","event":"command","ts":"2026-07-31T12:04:07Z"} +{"delta":"+5.0","event":"confidence","score":5,"trigger":"calc-score","ts":"2026-07-31T12:05:42Z"} +{"delta":"+0.0","event":"confidence","score":5,"trigger":"calc-score","ts":"2026-07-31T12:07:03Z"} +{"cmd":"fab-fff","event":"command","ts":"2026-07-31T12:07:26Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T12:07:36Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T12:22:22Z"} +{"event":"review","result":"failed","ts":"2026-07-31T12:36:23Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T12:36:23Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T12:43:42Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-07-31T12:55:18Z"} +{"event":"review","result":"passed","ts":"2026-07-31T12:55:18Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T12:56:14Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T13:04:16Z"} +{"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-31T13:06:40Z"} +{"event":"review","result":"passed","ts":"2026-07-31T13:23:21Z"} diff --git a/fab/changes/260731-xkfl-correct-false-stale-claims/.status.yaml b/fab/changes/260731-xkfl-correct-false-stale-claims/.status.yaml new file mode 100644 index 0000000..7f8f616 --- /dev/null +++ b/fab/changes/260731-xkfl-correct-false-stale-claims/.status.yaml @@ -0,0 +1,53 @@ +id: xkfl +name: 260731-xkfl-correct-false-stale-claims +created: 2026-07-31T12:04:07Z +created_by: droid-ash +change_type: docs +issues: [] +progress: + intake: done + apply: done + review: done + hydrate: done + ship: done + review-pr: done +plan: + generated: true + task_count: 11 + acceptance_count: 14 + acceptance_completed: 14 +confidence: + certain: 8 + confident: 1 + tentative: 0 + unresolved: 0 + score: 5.0 + fuzzy: true + dimensions: + signal: 87.2 + reversibility: 87.8 + competence: 90.6 + disambiguation: 84.4 +stage_metrics: + intake: {started_at: "2026-07-31T12:04:07Z", driver: fab-new, iterations: 1, completed_at: "2026-07-31T12:07:36Z"} + apply: {started_at: "2026-07-31T12:36:23Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T12:43:42Z"} + review: {started_at: "2026-07-31T12:43:42Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T12:55:18Z"} + hydrate: {started_at: "2026-07-31T12:55:18Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T13:04:16Z"} + ship: {started_at: "2026-07-31T13:04:16Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T13:06:40Z"} + review-pr: {started_at: "2026-07-31T13:06:40Z", driver: git-pr, iterations: 1, completed_at: "2026-07-31T13:23:21Z"} +prs: + - https://github.com/droid-ash/finalrun-agent/pull/176 +change_type_source: explicit +true_impact: + added: 609 + deleted: 51 + net: 558 + tests: + added: 4 + deleted: 4 + net: 0 + computed_at: "2026-07-31T13:06:40Z" + computed_at_stage: ship +summary: 'Corrected false and stale factual claims across fab policy, source comments, the codebase walkthrough and memory: dropped a fabricated drivers.yml comment ratio, past-tensed the shipped restatement sweep, repointed twice-rotted logger.ts citations to line-free anchors, replaced five used-by attributions naming nonexistent Dart-predecessor symbols, and re-attributed the streaming fps defaults (gRPC StartStreaming 24, legacy WebSocket 1).' +# true_impact: lazily created on first stage-finish that computes it (no placeholder here). +last_updated: 2026-07-31T13:23:21Z diff --git a/fab/changes/260731-xkfl-correct-false-stale-claims/intake.md b/fab/changes/260731-xkfl-correct-false-stale-claims/intake.md new file mode 100644 index 0000000..f2f598e --- /dev/null +++ b/fab/changes/260731-xkfl-correct-false-stale-claims/intake.md @@ -0,0 +1,213 @@ +# Intake: Correct False and Stale Factual Claims + +**Change**: 260731-xkfl-correct-false-stale-claims +**Created**: 2026-07-31 + +## Origin + +One-shot `/fab-new` invocation with a fully-specified item list. The items come from an +independent adversarial review of PRs #168–#175, re-verified by the requester against main at +`91b2683`. The requester explicitly warned that **line numbers rot** — every location below must +be re-derived at apply time, never trusted from the numbers given. Raw input (abridged only where +it repeats these instructions): + +> Correct false and stale factual claims across comments, docs and policy. This change is +> documentation and comment content only: do NOT change any runtime behaviour, do not rename +> anything, do not refactor. It is also the change that fixes errors THIS pipeline introduced, so +> accuracy matters more than speed. [Items 1–8 plus nice-to-haves — reproduced in full under +> "What Changes" below.] Finally, do NOT commit or git add fab/backlog.md — it is intentionally +> untracked reference scratch; note the git-pr expected-area guard would otherwise stage untracked +> files under fab/. + +## Why + +1. **Problem**: eight clusters of factual claims in comments, docs, and fab policy files are + false or stale — some were false when written (fabricated/mislabelled measurements, citations + of rules that say the opposite, used-by headers naming symbols that never existed in this + repo), others rotted when later PRs moved the ground under them (line-number citations, a + "planned" sweep that already shipped, a bookkeeping record for a merged change stuck at + `review-pr: active`). +2. **Consequence if unfixed**: these files are the project's stated source of truth — policy + files feed every future apply/review pass, and memory docs are declared authoritative + (`docs/memory/index.md`). False claims propagate: an agent reading `code-quality.md` would cite + a fabricated 67/18 ratio as precedent; one reading `code-review.md` might re-run a sweep that + already merged as PR #171; the stuck `.status.yaml` makes a completed change look in-flight. + Several of these errors were introduced by this very pipeline, so leaving them standing + compounds the credibility cost. +3. **Approach**: a single docs/comments-only truth pass. Every correction is re-verified against + the current tree before editing (measure counts, grep for symbols, read the code the citation + points at). No runtime behaviour changes, no renames, no refactors. Accuracy over speed. + +## What Changes + +### Item 1 — `fab/project/code-quality.md` (§ Comments, CI-and-workflow paragraph, ~line 38) + +The paragraph presents `.github/workflows/drivers.yml` as the canonical fully-compliant example +and cites "67 comment lines against 18 functional lines". **The figure is false and was never +true of the file**: measured, the file is 103 comment / 44 functional lines, identical at the +commit that wrote the claim. The 67/18 came from a mid-flight snapshot of an in-progress diff +(which had reached 69/17 by merge) — a diff measurement mislabelled as a property of the file. +The paragraph also self-contradicts: it states there is *no comment-to-code ratio cap because +density is not the test*, then offers a ratio as proof of compliance. + +**Fix**: remove the numeric ratio entirely and let the deletion test carry the argument (the +requester's preferred option). If any figure is kept, it must state exactly what it measures and +be re-verified against the current file. Preserve the rest of the paragraph's content (ruleset +`14531661` / `build.gradle.kts:104` citations, the drivers.yml-vs-ci.yml paths-filter rationale). + +### Item 2 — `fab/project/code-review.md` (§ Project-Specific Review Rules, ~line 60) + +The sweep-scope bullet calls the restatement-comment sweep (~146 audit findings) "planned" and +"executed as a separate later change". That sweep **already shipped as PR #171** +(`260731-vxq1-comment-content-sweep`, merged commit `7b38afc`). **Fix**: rewrite to past tense +and reference the merged change/PR so nobody re-runs it. The scope rules themselves (rationale +claims out of scope, mixed-block handling) remain valid and stay. + +### Item 3 — `packages/common/src/env.ts` (~line 9, re-export shim comment) + +The comment claims the constitution's Test Integrity rule *forbids* editing the test to chase a +new import path. `fab/project/constitution.md` line 14 says the opposite on both halves: it +explicitly **permits** updating tests to match the spec, and it prohibits modifying +*implementation* code solely to accommodate test infrastructure — which is arguably what +retaining the re-export shim does. PR #172 also edited that very test file (added 5 tests), +disproving the comment's premise. **Fix**: correct the citation so the comment states the real +reason the shim is retained — consumer compatibility, *if verification confirms that is the +reason* (check actual importers of `env.ts` before writing it). Do not misattribute it to the +constitution. The shim itself stays (removing it would be a runtime change — out of scope). + +### Item 4 — citation rot: `logger.ts:103-105` → current location + +Two places cite `packages/common/src/logger.ts:103-105` for the unguarded sink loop: + +- `packages/device-node/src/device/logWriteStream.ts` (~line 89) — note the request's path typo; + this is the correct path, verified to exist +- `docs/memory/device-node/log-capture.md` (~line 167) + +The loop is now at `logger.ts:99` (PR #171 deleted four one-line comments above it). The +substantive claim — the loop is unguarded — is **still true**; only the line reference rotted. +**Fix**: repoint both citations, and prefer a line-number-free anchor (e.g., function/method name +in `logger.ts`) since this exact citation has now rotted twice. Verify the loop's current +location and its unguardedness before rewriting. + +### Item 5 — false used-by attributions naming symbols that do not exist + +Five headers name consumers that exist nowhere in the codebase (they are Dart predecessor names — +`AIAgent.ts` line 1 says it replaces `FinalRunAgent.dart`): + +| File | ~Loc | False symbol(s) | +|------|------|-----------------| +| `packages/common/src/constants.ts` | line 11 | `FinalRunAgent` | +| `packages/common/src/constants.ts` | line 153 | `HeadlessGoalExecutor` | +| `packages/common/src/models/Hierarchy.ts` | lines 2–3 | `FinalRunAgent`, `HeadlessActionExecutor` | +| `drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/DriverServiceImpl.kt` | ~line 39 | "replaces `ActionProcessor`" | +| `drivers/android/app/src/androidTest/java/app/finalrun/android/grpc/GrpcDriverServer.kt` | ~line 13 | `WebSocketServerImpl` | + +Path corrections verified: `DriverServiceImpl.kt` (not `.ts`), and **both** Kotlin files live +under `androidTest/` (the request said `grpc/GrpcDriverServer.kt` without a tree prefix; +`git ls-files` confirms `androidTest`). PR #170 deleted a fourth identical false header citing +exactly this falseness; PR #171 then edited both TS files and left these standing. + +**Fix**: replace each false name with the real consumers, verified by grep at apply time. +Candidates named by the requester: goal-executor `AIAgent.ts`, `ai/schemas.ts`, +`ActionExecutor.ts`, `TestExecutor.ts` — verify, do not copy blindly. For the Kotlin files, +verify against actual referencing code (or drop the "replaces X" claim if no real predecessor +exists in-repo). + +### Item 6 — `docs/memory/drivers/grpc-contract.md` (~lines 128–130, fps default) + +The doc claims an omitted `fps` defaults to **1** and that the proto-documented **24** "is not +adopted". Per the requester (verify before rewriting): the gRPC `StartStreaming` path actually +defaults to **24** — see `GrpcDriverServer.swift` and `proto/finalrun/driver.proto` — and the +**1** belongs to the legacy WebSocket path. The doc contradicts both the code and the code's own +comment. **Fix**: rewrite the passage to attribute each default to its correct path, after +reading the Swift implementation and the proto. + +### Item 7 — `docs/codebase-walkthrough.md` (pre-existing, five false claims) + +Never touched by the recent work; carries: + +1. "5 packages" when there are 7 (omits `cloud-core` and `local-runtime`) +2. A wrong `runTestCommand` line range +3. "18 action types" when there are 22 +4. A table headed `packages/cli/src` listing six files that actually live in + `packages/common/src`, with two functions miscredited +5. A path `packages/cli/src/checkRunner.ts` that does not exist (file is under + `packages/common/src`) + +**Fix**: correct all five, each re-verified with `find`/`git ls-files`/direct reads — counts +counted, line ranges re-derived, table paths and function attributions checked against the +actual files. + +### Item 8 — pipeline bookkeeping: `fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml` + +Records `review-pr` as `active` with no `completed_at`, though the change merged as PR #170 (its +finishing commit was made locally after merge and never pushed). **Fix**: complete the record so +it reflects a finished pipeline. Prefer fab tooling with the change-name override +(e.g., `fab status finish 3vhw review-pr` — verify the exact stage state first with +`fab preflight 3vhw` / `fab status`); fall back to a careful direct YAML edit only if the tooling +refuses from this worktree. + +### Nice-to-haves (do if cheap and verifiable, else skip) + +- `packages/common/src/constants.ts` line 1: "only the subset used by CLI plus goal-executor plus + device-node" — marginally overstated since PR #172 removed `env.ts`'s import of + `REASONING_LEVELS`. Reword if the verified consumer set makes it easy. +- `packages/common/src/models/Hierarchy.ts` line 2: claims the Dart file is ~108KB — unverifiable + (no Dart sources in this repo or its history). Drop the figure or mark it unverifiable. + +### Constraints (apply to every item) + +- **Docs and comment content only** — no runtime behaviour changes, no renames, no refactors. +- **Re-derive every location** — line numbers in this intake are hints, not addresses. +- **Verify before writing** — every replacement claim must be checked against the current tree; + this change exists because unverified claims were written before. +- **Never commit or `git add` `fab/backlog.md`** — intentionally untracked reference scratch. + (It does not currently exist in this worktree, but the git-pr expected-area guard stages + untracked files under `fab/`, so the ship stage must exclude it explicitly if it appears.) + +## Affected Memory + +- `device-node/log-capture`: (modify) repoint the rotted `logger.ts:103-105` citation (Item 4) — + content correction within the memory file itself +- `drivers/grpc-contract`: (modify) correct the fps-default attribution (gRPC StartStreaming = 24 + from proto; legacy WebSocket path = 1) per Item 6 + +No other memory files change: the remaining edits are source comments, fab policy files, +`docs/codebase-walkthrough.md`, and one `.status.yaml` — none alter spec-level behavior. + +## Impact + +- **fab policy**: `fab/project/code-quality.md`, `fab/project/code-review.md` — feed every future + apply/review pass +- **Source comments** (content only): `packages/common/src/env.ts`, `constants.ts`, + `models/Hierarchy.ts`; `packages/device-node/src/device/logWriteStream.ts`; + `drivers/android/.../androidTest/.../grpc/DriverServiceImpl.kt`, `GrpcDriverServer.kt` +- **Docs**: `docs/memory/device-node/log-capture.md`, `docs/memory/drivers/grpc-contract.md`, + `docs/codebase-walkthrough.md` +- **Pipeline state**: `fab/changes/260731-3vhw-.../.status.yaml` +- **Zero runtime impact**: no executable line changes; tests unaffected. Verification work reads + `logger.ts`, `AIAgent.ts`, `ai/schemas.ts`, `ActionExecutor.ts`, `TestExecutor.ts`, + `GrpcDriverServer.swift`, `proto/finalrun/driver.proto`, and the package tree, but does not + modify them (except where an item explicitly targets them). + +## Open Questions + +- None — the request is fully specified, prescribes fix strategies per item, and grants + discretion explicitly where wanted (nice-to-haves "if cheap and verifiable"; Item 4's durable + anchor "consider"). + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | Item 1: remove the drivers.yml numeric ratio entirely rather than correcting it to 103/44 | Requester prescribed this as the preferred fix ("remove the numeric ratio entirely and let the deletion test carry the argument"); keeping a figure is allowed only with verified provenance | S:95 R:90 A:95 D:90 | +| 2 | Confident | Item 3: the shim-retention reason to write is consumer compatibility, contingent on verifying actual importers of `env.ts` at apply time | Requester hedged ("if that is the reason"); agent can resolve it mechanically by grepping importers before writing | S:80 R:85 A:80 D:70 | +| 3 | Confident | Item 4: switch both citations to a line-number-free anchor (function-level reference) instead of just repointing to `logger.ts:99` | Requester said "consider whether a line-number-free reference would be more durable, since this exact citation has now rotted twice" — strong signal toward durability; trivially reversible | S:75 R:90 A:85 D:75 | +| 4 | Certain | Item 5: both Kotlin files (`DriverServiceImpl.kt`, `GrpcDriverServer.kt`) live under `androidTest/`, and the `.ts`→`.kt` extension correction applies | Verified via `git ls-files` during intake; request's own path hints were partially wrong | S:90 R:95 A:100 D:95 | +| 5 | Confident | Item 5: replacement used-by names come from apply-time grep, using the requester's candidates (AIAgent.ts, ai/schemas.ts, ActionExecutor.ts, TestExecutor.ts) as hypotheses only | Requester: "which you must verify yourself"; grep is deterministic | S:85 R:85 A:90 D:80 | +| 6 | Certain | Item 8: use fab tooling (change-name override) to complete 3vhw's record; hand-edit `.status.yaml` only as fallback | Requester prescribed exactly this preference order | S:95 R:80 A:90 D:90 | +| 7 | Confident | Do both nice-to-haves (constants.ts line 1 wording, Hierarchy.ts 108KB figure) since both are one-line comment edits verifiable at apply time | "If cheap and verifiable" — both are; Hierarchy.ts figure handled by dropping or marking unverifiable per the stated options | S:80 R:90 A:85 D:75 | +| 8 | Certain | Ship stage must never stage `fab/backlog.md`; it is absent in this worktree today but the guard binds if it appears | Explicit user constraint; absence verified during intake | S:95 R:85 A:95 D:95 | +| 9 | Certain | Change type is `docs` (comment/doc content only, plus one bookkeeping YAML), despite "fix" wording in the description | The change alters no runtime behaviour by explicit constraint | S:90 R:90 A:95 D:90 | + +9 assumptions (5 certain, 4 confident, 0 tentative, 0 unresolved). diff --git a/fab/changes/260731-xkfl-correct-false-stale-claims/plan.md b/fab/changes/260731-xkfl-correct-false-stale-claims/plan.md new file mode 100644 index 0000000..fab0df1 --- /dev/null +++ b/fab/changes/260731-xkfl-correct-false-stale-claims/plan.md @@ -0,0 +1,244 @@ +# Plan: Correct False and Stale Factual Claims + +**Change**: 260731-xkfl-correct-false-stale-claims +**Intake**: `intake.md` + +## Requirements + +### Fab Policy: comment-policy example must not carry a fabricated measurement + +#### R1: Remove the false 67/18 ratio from code-quality.md +The CI-and-workflow paragraph of `fab/project/code-quality.md` `## Comments` MUST NOT cite any +comment-to-functional line ratio for `.github/workflows/drivers.yml`. The deletion-test argument +carries the example alone. The retained citations (ruleset `14531661`, `build.gradle.kts:104`, +the drivers.yml-vs-ci.yml paths-filter rationale) MUST be preserved and are verified to still +exist in `drivers.yml` (lines 21/29/42 cite the ruleset; line 52 cites `build.gradle.kts:104`). + +- **GIVEN** the current paragraph citing "67 comment lines against 18 functional lines" +- **WHEN** the paragraph is rewritten +- **THEN** no numeric line-count or ratio claim about drivers.yml remains, and the ruleset / + gradle citations and paths-filter rationale survive intact + +### Fab Policy: sweep-scope bullet must reflect the shipped sweep + +#### R2: Rewrite the "planned" sweep bullet in code-review.md to past tense +The sweep-scope bullet in `fab/project/code-review.md` `## Project-Specific Review Rules` MUST +state that the restatement-comment sweep already shipped (change `260731-vxq1-comment-content-sweep`, +PR #171, merged commit `7b38afc`) so no future agent re-runs it. The scope rules themselves +(rationale claims out of scope, mixed-block handling, CI/workflow coverage) MUST remain. + +- **GIVEN** the bullet describing a "planned" sweep "executed as a separate later change" +- **WHEN** it is rewritten +- **THEN** it references the merged change/PR in past tense and keeps the scope rules + +### Source Comments: citations must state verified reasons and name real symbols + +#### R3: Correct the env.ts shim comment's constitution misattribution +The re-export shim comment in `packages/common/src/env.ts` (currently lines 7–9) MUST NOT claim +the constitution's Test Integrity rule forbids editing tests (constitution line 14 explicitly +permits updating tests to match the spec). The comment SHALL state the verified retention +reason: consumer compatibility — `packages/common/src/test/env.test.ts` still imports +`parseModel`/`parseReasoningLevel` through `../env.js`, and the shim keeps that import path +working. The export block itself MUST NOT change. + +- **GIVEN** the comment citing the constitution as forbidding a test edit +- **WHEN** it is corrected +- **THEN** it states the consumer-compatibility reason without citing the constitution, and the + `export { ... } from './constants.js'` statement is byte-identical + +#### R4: Repoint the two rotted logger.ts:103-105 citations with durable anchors +Both citations of `packages/common/src/logger.ts:103-105` — in +`packages/device-node/src/device/logWriteStream.ts` (~line 89) and +`docs/memory/device-node/log-capture.md` (~line 167) — MUST be repointed to the loop's current +location using a line-number-free anchor (the sink loop in `Logger._emit`, +`packages/common/src/logger.ts`; verified currently at lines 99–101, still unguarded). The +adjacent `packages/cli/src/reportWriter.ts:132` citation in the same sentences SHOULD likewise +become line-free (`ReportWriter.createLoggerSink()` is now at line 128 — that citation has +rotted too). + +- **GIVEN** the two comments citing `logger.ts:103-105` +- **WHEN** they are rewritten +- **THEN** each cites `Logger._emit`'s sink loop by name without line numbers, the unguardedness + claim is preserved (verified true), and no other content of the blocks changes + +#### R5: Replace used-by attributions naming symbols that do not exist in this repo +Five headers MUST stop naming Dart-predecessor symbols that exist nowhere in the codebase, and +name grep-verified real consumers instead: + +1. `packages/common/src/constants.ts` (~line 11, AI feature names): `FinalRunAgent` → the real + consumers (goal-executor's `AIAgent.ts`/`VisualGrounder.ts`/`ActionExecutor.ts`/`ai/schemas.ts` + select prompts/models per feature; `workspace.ts` keys per-feature config overrides). +2. `packages/common/src/constants.ts` (~line 153, planner action keys): `HeadlessGoalExecutor` → + the real consumers (`AIAgent.ts` normalizes the planner response onto these keys; + `ActionExecutor.ts`/`TestExecutor.ts` route on them). The adjacent sentence "These must match + the strings the planner LLM outputs" MUST also be corrected: verified false — the planner + emits snake_case `action_type` strings which `AIAgent`'s `FIXED_PROMPT_ACTIONS` map normalizes + onto these keys. +3. `packages/common/src/models/Hierarchy.ts` (lines 2–3): `FinalRunAgent`/`HeadlessActionExecutor` + → verified consumers (goal-executor's `AIAgent.ts`, `ActionExecutor.ts`, `TestExecutor.ts`, + plus device-node). +4. `drivers/android/.../androidTest/.../grpc/DriverServiceImpl.kt` (~line 39): drop + "replaces ActionProcessor" — no `ActionProcessor` exists in-repo (grep + `git log -S` show + only this comment); keep the true "handles all incoming RPC calls from packages/device-node". +5. `drivers/android/.../androidTest/.../grpc/GrpcDriverServer.kt` (~line 13): drop + "replaces WebSocketServerImpl" — same verification; keep the true server description. + +- **GIVEN** each header naming a nonexistent symbol +- **WHEN** it is rewritten +- **THEN** every consumer named is grep-verifiable in the current tree and no executable line + changes + +### Memory: fps defaults attributed to the correct paths + +#### R6: Correct the fps-default attribution in grpc-contract.md +`docs/memory/drivers/grpc-contract.md` (fps requirement section, ~lines 127–130) MUST state: +an omitted `fps` on the gRPC `StartStreaming` path defaults to **24** +(`GrpcDriverServer.swift` `defaultStreamingFps = 24`, adopting `driver.proto`'s documented +`// Default: 24`), while `XCViewHierarchyManager`'s `defaultStreamingFps` of **1** defaults the +legacy WebSocket timer path — deliberately not aligned, since aligning them would change RPC +behaviour rather than guard arithmetic. The clamp content (1–60) stays. + +- **GIVEN** the passage claiming an omitted fps defaults to 1 and "the proto's documented 24 is + not adopted" +- **WHEN** it is rewritten after reading `GrpcDriverServer.swift` and `driver.proto` +- **THEN** each default is attributed to its correct path, matching the Swift code and its + comments (verified: `GrpcDriverServer.swift:130-137,651`; `XCViewHierarchyManager.swift:39,64`; + `driver.proto:170`) + +### Docs: codebase walkthrough factual corrections + +#### R7: Correct the five false claims in docs/codebase-walkthrough.md +1. "monorepo with 5 packages" → **7** (adds `cloud-core`, `local-runtime` — verified by + `ls packages/`); the architecture tree and the "Why 5 packages?" heading/table gain the two + missing packages with descriptions verified from their `package.json`. +2. `runTestCommand()` line range → re-derived **`bin/finalrun.ts:372-485`** (verified by + definition line + brace balance). +3. "(18 types)" for `DeviceAction.ts` → **22** concrete action classes (counted). +4. The `packages/cli/src/` Package Map table: the six rows whose files live in + `packages/common/src/` (`checkRunner.ts`, `testLoader.ts`, `testSelection.ts`, `workspace.ts`, + `appConfig.ts`, `env.ts`) are moved/re-labelled to the correct package; the two miscredited + functions are fixed (`parseModel()` is defined in `constants.ts` and only re-exported through + env; `resolveApiKey()` lives in `packages/cli/src/apiKey.ts`); rows citing files that no + longer exist anywhere (`reportTemplate.ts`, `reportIndexTemplate.ts` — verified absent) are + corrected or removed. +5. `packages/cli/src/checkRunner.ts` (§4 File header) → `packages/common/src/checkRunner.ts`. + +- **GIVEN** each claim +- **WHEN** corrected +- **THEN** every count/path/range/attribution written matches a fresh measurement of the tree + +### Pipeline State: 3vhw bookkeeping completed + +#### R8: Complete 3vhw's review-pr record +`fab/changes/260731-3vhw-delete-dead-code-audit-targets/.status.yaml` MUST record `review-pr` +as `done` with a `completed_at`, reflecting merged PR #170. Preferred mechanism: fab tooling with +the change-name override (`fab status finish 3vhw review-pr`; state pre-verified via +`fab preflight 3vhw` → `review-pr: active`); direct YAML edit only if the tooling refuses. + +- **GIVEN** `progress.review-pr: active` with no `completed_at` +- **WHEN** the finish command runs +- **THEN** `progress.review-pr: done` and `stage_metrics.review-pr.completed_at` is set + +### Nice-to-haves + +#### R9: constants.ts header wording and Hierarchy.ts unverifiable figure +1. `packages/common/src/constants.ts` line 1: replace the "CLI + goal-executor + device-node" + enumeration (marginally stale) with a claim verified at apply time (the subset this repo's + TypeScript packages actually use). +2. `packages/common/src/models/Hierarchy.ts` line 2: drop the "~108KB" figure (no Dart sources + exist in this repo or its history to verify it). + +- **GIVEN** the two comments +- **WHEN** reworded +- **THEN** neither carries an unverifiable or stale claim + +### Non-Goals + +- No runtime behaviour changes, renames, or refactors — comment/doc/YAML content only. +- The wider rot discovered in `docs/codebase-walkthrough.md`'s report-web section (report-web is + now a Vite React SPA, not Next.js; its Package Map table lists files that no longer exist) is + corrected ONLY where the five in-scope claims force an edit to the same block; the rest is + reported, not fixed. +- `fab/backlog.md` is never created, staged, or committed (currently absent — verified). + +## Tasks + +### Phase 1: Setup + +*(none — verification sweep performed at plan generation; evidence recorded in Requirements)* + +### Phase 2: Core Implementation + +- [x] T001 [P] Rewrite the drivers.yml example sentence in `fab/project/code-quality.md` § Comments to drop the 67/18 ratio, preserving ruleset/gradle citations and paths-filter rationale +- [x] T002 [P] Rewrite the sweep-scope bullet in `fab/project/code-review.md` § Project-Specific Review Rules to past tense referencing PR #171 / `260731-vxq1-comment-content-sweep` +- [x] T003 [P] Correct the shim comment in `packages/common/src/env.ts` to the verified consumer-compatibility reason; no executable change +- [x] T004 [P] Repoint the `logger.ts:103-105` citation in `packages/device-node/src/device/logWriteStream.ts` to a line-free `Logger._emit` anchor (and line-free `createLoggerSink` reference) +- [x] T005 [P] Repoint the same citation in `docs/memory/device-node/log-capture.md` identically; re-check the file's `description:` still routes +- [x] T006 [P] Replace the five false used-by attributions (constants.ts ×2, Hierarchy.ts, DriverServiceImpl.kt, GrpcDriverServer.kt) with grep-verified consumers; correct the adjacent false "must match the strings the planner LLM outputs" sentence +- [x] T007 [P] Rewrite the fps-default passage in `docs/memory/drivers/grpc-contract.md` attributing 24 to gRPC `StartStreaming` and 1 to the legacy WebSocket path +- [x] T008 [P] Correct the five walkthrough claims in `docs/codebase-walkthrough.md` (package count 7, `runTestCommand` 372-485, 22 action types, Package Map table paths/functions, checkRunner path) +- [x] T009 Complete 3vhw's record via `fab status finish 3vhw review-pr` (state pre-verified `active`) +- [x] T010 [P] Apply the two nice-to-have rewordings (constants.ts line 1, Hierarchy.ts ~108KB) + +### Phase 3: Integration & Edge Cases + +- [x] T011 Verify zero runtime impact: `git diff` inspection confirms only comment/doc/YAML lines changed (no executable-line change in any `.ts`/`.kt` file), and `fab/backlog.md` remains absent/unstaged + +## Acceptance + +### Functional Completeness + +- [x] A-001 R1: `fab/project/code-quality.md` carries no numeric ratio for drivers.yml; ruleset `14531661` and `build.gradle.kts:104` citations and the paths-filter rationale remain — verified: `drivers.yml:21,29,42` cite the ruleset, `:52` cites `build.gradle.kts:104`, `:40-45` carry the paths-filter-vs-ci.yml rationale +- [x] A-002 R2: the code-review.md sweep bullet is past tense and names the merged change/PR — verified `7b38afc` = "docs: Comment Content Sweep (#171)" and `fab/changes/260731-vxq1-comment-content-sweep` exists +- [x] A-003 R3: env.ts shim comment states consumer compatibility, cites no constitution prohibition; exports unchanged — verified `packages/common/src/test/env.test.ts:6` imports `parseModel`/`parseReasoningLevel` from `../env.js`; export block byte-identical in the diff +- [x] A-004 R4: both citations anchor to `Logger._emit`'s sink loop without line numbers; unguardedness claim retained — verified `logger.ts:99-101` loop has no try/catch, `reportWriter.ts:128` `createLoggerSink()` is a bare `fs.appendFileSync`, installed via `Logger.addSink` at `testRunner.ts:317` +- [x] A-005 R5: none of the five headers names `FinalRunAgent`, `HeadlessGoalExecutor`, `HeadlessActionExecutor`, `ActionProcessor`, or `WebSocketServerImpl`; every named consumer greps in-tree — re-verified independently after rework: the five false symbols are gone from all five headers (the only surviving `FinalRunAgent` hits are `AIAgent.ts:1,199,201`, all naming the *Dart* file `FinalRunAgent.dart` — out of scope and true). constants.ts:11-14 verifies (`AIAgent.ts:23-29,321,891-903` and `VisualGrounder.ts:7,51` select prompts/models per feature; `ai/schemas.ts:194-200` keys `FEATURE_SCHEMAS`; `workspace.ts:7,80,467` keys overrides off `ALL_FEATURES`). constants.ts:156-159 verifies (`AIAgent.ts:1142-1155` `FIXED_PROMPT_ACTIONS` maps snake_case `action_type` → `PLANNER_ACTION_*`; `ActionExecutor.ts:210-233` handler map; `TestExecutor.ts:562,590` terminals). **Rework finding resolved**: `Hierarchy.ts:2-4` no longer names `device-node` (confirmed zero `Hierarchy`/`HierarchyNode` references there) and now names four verified importers — `AIAgent.ts:22`, `ActionExecutor.ts:6`, `TestExecutor.ts:7`, `GrounderResponseConverter.ts:5` +- [x] A-006 R6: grpc-contract.md attributes 24 to gRPC StartStreaming (proto-adopted) and 1 to the legacy WebSocket path — verified `GrpcDriverServer.swift:130-137,651`, `XCViewHierarchyManager.swift:39,64`, `driver.proto:170` (Android `DriverServiceImpl.kt:494` also defaults 24) +- [x] A-007 R7: walkthrough says 7 packages, `finalrun.ts:372-485`, 22 types, table paths/functions match `git ls-files` and real definitions, checkRunner path is `packages/common/src` — independently re-measured after rework: `ls packages/` = 7 (`cloud-core`/`local-runtime` descriptions match their `package.json:5`); `runTestCommand` spans 372→485 (brace-balanced, next decl at 487); 22 concrete `DeviceAction` subclasses; `parseModel` defined `constants.ts:92`, `parseReasoningLevel` `constants.ts:44`, `resolveApiKey` in `cli/src/apiKey.ts:3`, every moved row's file+function verified (`runCheck` `checkRunner.ts:52`, `loadTest` `testLoader.ts:86`, `loadWorkspaceConfig` `workspace.ts:431`, `CliEnv` `env.ts:29`, …); `reportTemplate.ts`/`reportIndexTemplate.ts` absent from `git ls-files`; §4 header now `packages/common/src/checkRunner.ts`. **Rework findings resolved**: (must-fix) `:38` and `:62` now state report-web is served by the CLI's report server and is a build-time dependency — verified `cli/src/reportServer.ts:10,23-36`, `cli/scripts/copyReportApp.mjs:14-28` (bails), `local-runtime/scripts/buildRuntimeTarball.mjs:130-134` (bails); (should-fix) `bin/finalrun.ts` moved to its own `### packages/cli/bin/` subsection (`:1117-1121`), `:1100` now reads "(22 concrete action classes)", and §10.1 `:682,685` now read "React SPA (`report-web`)" / "Vite + React SPA" +- [x] A-008 R8: 3vhw `.status.yaml` shows `review-pr: done` with `completed_at` — `completed_at: "2026-07-31T12:20:33Z"` +- [x] A-009 R9: constants.ts line 1 and Hierarchy.ts line 2 carry no stale/unverifiable claims — `~108KB` dropped; enumeration replaced with an enumeration-free claim + +### Behavioral Correctness + +- [x] A-010 R3 R5: no executable line changed in any edited `.ts`/`.kt` file (git diff shows comment-only hunks) — verified mechanically: every added/removed line in `*.ts`/`*.kt` starts with `//`, `*`, or `/*` + +### Scenario Coverage + +- [x] A-011 R8: `fab preflight 3vhw` (or `fab status`) reflects a fully-done pipeline after the finish — `display_stage: review-pr`, `display_state: done`, all six stages `done` + +### Edge Cases & Error Handling + +- [x] A-012 R7: every replacement figure in the walkthrough was re-measured at apply time, not copied from the intake — independently re-measured during review; note the intake's own 103/44 drivers.yml figure was correctly NOT carried into any file + +### Code Quality + +- [x] A-013 Pattern consistency: rewritten comments follow `code-quality.md` § Comments (rationale-only, deletion-test-passing; no new restatement comments) — each rewritten header carries cross-file consumer coupling or retention rationale, none recoverable from the adjacent code +- [x] A-014 No unnecessary duplication: corrections edit existing blocks in place; no parallel/duplicate explanation added + +## Notes + +- Check items as you review: `- [x]` +- All acceptance items must pass before `/fab-continue` (hydrate) +- `fab/backlog.md` must never be staged or committed (absent in this worktree; guard binds at ship) + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | R1: remove the ratio with no replacement figure | Requester's preferred option; measurement is method-sensitive (this run's stripped-`#` count gives 138/53, not the intake's 103/44), so any written figure would invite the same rot | S:95 R:90 A:95 D:90 | +| 2 | Certain | R5: drop the Kotlin "replaces X" clauses instead of substituting a predecessor | `ActionProcessor`/`WebSocketServerImpl` exist nowhere in-repo (grep + `git log -S` hit only these comments); intake explicitly allows dropping when no real predecessor exists | S:90 R:90 A:95 D:90 | +| 3 | Confident | R5: also correct the adjacent "must match the strings the planner LLM outputs" sentence in constants.ts | Verified false (AIAgent's `FIXED_PROMPT_ACTIONS` maps snake_case prompt strings onto these keys); leaving a verified-false claim in a header this change edits contradicts the change's purpose | S:70 R:90 A:90 D:75 | +| 4 | Confident | R4: convert the adjacent `reportWriter.ts:132` citation to line-free in the same sentences | `createLoggerSink` is now at line 128 — the citation has already rotted; same durability rationale the requester gave for the logger anchor | S:70 R:90 A:90 D:80 | +| 5 | Confident | R7: correct/remove the `reportTemplate.ts`/`reportIndexTemplate.ts` rows while fixing the cli/src table | Claim 4's fix is "table paths checked against the actual files"; both files verified absent from the tree, so leaving the rows fails the check the fix prescribes | S:65 R:90 A:90 D:70 | +| 6 | Confident | R7: in the two blocks the package-count fix edits (tree + Why table), label report-web accurately as a Vite React SPA; leave the report-web Package Map table untouched and report the rot | report-web verified non-Next.js (`vite` scripts, SPA description); wider walkthrough rot is out of the intake's five claims | S:60 R:85 A:85 D:65 | +| 7 | Confident | R7: keep a corrected numeric range (372-485) for `runTestCommand` rather than dropping line numbers | Intake prescribes "line ranges re-derived" for this item (unlike Item 4, where it prescribes durable anchors); walkthrough style uses ranges | S:75 R:90 A:85 D:75 | +| 8 | Confident | R9: reword constants.ts line 1 to "the subset this repo's TypeScript packages actually use" without enumerating packages | Consumer set spans more than the three named packages (common itself, cloud-core/report-web via barrel); an enumeration-free claim is verifiable and rot-proof | S:70 R:90 A:85 D:75 | +| 9 | Certain | R9: drop the ~108KB figure outright | Intake offers "drop or mark unverifiable"; no Dart sources in repo or history, so dropping is the cleaner of the two offered options | S:85 R:95 A:90 D:85 | +| 10 | Certain | R8: `fab status finish 3vhw review-pr` (tooling path) | State pre-verified `review-pr: active` via `fab preflight 3vhw`; exactly the transition the tooling's finish performs | S:95 R:85 A:95 D:90 | +| 11 | Certain | R5 rework: name `GrounderResponseConverter` in the Hierarchy.ts header alongside AIAgent/ActionExecutor/TestExecutor (rework annotation made it optional) | Verified importer (`goal-executor/src/GrounderResponseConverter.ts:5` imports `HierarchyNode`); naming all four goal-executor consumers keeps the header complete after dropping the false device-node claim (device-node has zero `Hierarchy`/`HierarchyNode` references) | S:85 R:95 A:95 D:90 | +| 12 | Confident | R7 rework: move the `bin/finalrun.ts` row into a new `### packages/cli/bin/` subsection rather than relabeling it inside the `cli/src` table | Rework annotation allows "move/fix"; a dedicated subsection keeps every table's File column relative to its heading, matching the doc's existing convention | S:75 R:90 A:90 D:80 | +| 13 | Confident | R7 rework: in §10.1 fix only the two Next.js labels (header + Technology row), leaving the section's remaining framing ("Two Report Server Implementations", "When used") untouched | Rework annotation scopes the fix to "those two labels"; the wider report-web rot stays reported-not-fixed per plan Non-Goals | S:70 R:90 A:90 D:75 | +| 14 | Confident | T011 rework: keep the ~146 figure in code-review.md but mark it as the source audit's never-independently-verified estimate (rather than dropping it) | Annotation offers "marked … or dropped"; marking preserves the scope-size context while removing the false precision | S:65 R:90 A:90 D:70 | +| 15 | Confident | T011 rework: cite Android's fps default via the line-free anchor `DriverServiceImpl.startStreaming` instead of `:494` | Verified `if (request.hasFps()) request.fps else 24` at that method; line-free anchors are the durability convention this change already adopted for the twice-rotted logger citation | S:80 R:90 A:90 D:85 | + +15 assumptions (5 certain, 10 confident, 0 tentative). diff --git a/fab/project/code-quality.md b/fab/project/code-quality.md index 25ee863..b229108 100644 --- a/fab/project/code-quality.md +++ b/fab/project/code-quality.md @@ -35,7 +35,7 @@ **Prohibition**: comments that restate what the code plainly says are prohibited. The decision procedure is the **deletion test**: if removing the comment leaves a competent reader able to recover everything it said from the code itself, it is restatement and must not be written. -**CI and workflow files**: non-obvious rationale comments in CI and workflow files (`.github/workflows/*.yml`, build scripts, and config files generally) are the desired use of comments — not merely tolerated — and are **exempt from any restatement sweep**. There is **no comment-to-code ratio cap**: density is not the test, recoverability is. The canonical positive example is `.github/workflows/drivers.yml` (PR #168): 67 comment lines against 18 functional lines, citing branch-protection ruleset `14531661` and `build.gradle.kts:104` to explain why a `paths` filter is safe on that workflow but would break `ci.yml` — every block passes the deletion test, so the file is fully compliant. Declarative files like workflow YAML have the *least* self-describing code and the *most* invisible external coupling, so they legitimately carry the highest comment density in the repo. +**CI and workflow files**: non-obvious rationale comments in CI and workflow files (`.github/workflows/*.yml`, build scripts, and config files generally) are the desired use of comments — not merely tolerated — and are **exempt from any restatement sweep**. There is **no comment-to-code ratio cap**: density is not the test, recoverability is. The canonical positive example is `.github/workflows/drivers.yml` (PR #168): its comment blocks cite branch-protection ruleset `14531661` and `build.gradle.kts:104` to explain why a `paths` filter is safe on that workflow but would break `ci.yml` — every block passes the deletion test, so the file is fully compliant. Declarative files like workflow YAML have the *least* self-describing code and the *most* invisible external coupling, so they legitimately carry the highest comment density in the repo. **Unit of judgement**: the individual claim, not the file or the block. A single block may mix both kinds — a rationale paragraph followed by a line restating the YAML key below it. Sweeping removes restatement sentences and keeps rationale sentences even when adjacent. diff --git a/fab/project/code-review.md b/fab/project/code-review.md index 9c35a45..14e82aa 100644 --- a/fab/project/code-review.md +++ b/fab/project/code-review.md @@ -57,5 +57,5 @@ Comment-content findings map as follows (policy in `code-quality.md` `## Comment - All user-facing strings must be internationalized --> - Comment content is governed by `code-quality.md` `## Comments`; the reviewer applies the deletion test per claim, not per block or file -- **Sweep scope**: the planned restatement-comment sweep (~146 audit findings, executed as a separate later change) targets restatement comments only. **Non-obvious rationale claims are out of the sweep's scope regardless of comment-to-code ratio**; in a mixed block, restatement claims remain in scope while adjacent rationale claims are kept. This explicitly covers CI/workflow files such as `.github/workflows/drivers.yml` and `ci.yml`, whose rationale blocks (ruleset IDs, cross-file couplings, measured timings, rejected alternatives) landed recently and deliberately +- **Sweep scope**: the restatement-comment sweep (the source audit's own estimate was ~146 findings — a figure never independently verified) already shipped as change `260731-vxq1-comment-content-sweep` (PR #171, merged commit `7b38afc`) — it must not be re-run. It targeted restatement comments only. **Non-obvious rationale claims were out of the sweep's scope regardless of comment-to-code ratio**; in a mixed block, restatement claims were in scope while adjacent rationale claims were kept. These scope rules still govern any future restatement flagging. This explicitly covers CI/workflow files such as `.github/workflows/drivers.yml` and `ci.yml`, whose rationale blocks (ruleset IDs, cross-file couplings, measured timings, rejected alternatives) landed recently and deliberately - **Ambiguity bias**: when a comment is arguably either kind, keep it. A false keep costs a few lines; a false delete costs unrecoverable context diff --git a/packages/common/src/constants.ts b/packages/common/src/constants.ts index b4ffcaf..153f88b 100644 --- a/packages/common/src/constants.ts +++ b/packages/common/src/constants.ts @@ -1,5 +1,5 @@ -// Port of constants/lib/constants.dart — only the subset used by -// CLI + goal-executor + device-node. +// Port of constants/lib/constants.dart — only the subset this repo's +// TypeScript packages actually use. // ============================================================================ // Platform identifiers @@ -8,7 +8,10 @@ export const PLATFORM_ANDROID = 'android'; export const PLATFORM_IOS = 'ios'; // ============================================================================ -// AI feature names — used by FinalRunAgent to select prompts/models +// AI feature names — used by the goal-executor (AIAgent, VisualGrounder, +// ActionExecutor) to select prompts/models per feature, by ai/schemas.ts to +// key FEATURE_SCHEMAS (the per-feature response schemas), and by workspace.ts +// to key per-feature config overrides // ============================================================================ export const FEATURE_PLANNER = 'planner'; export const FEATURE_GROUNDER = 'grounder'; @@ -150,8 +153,10 @@ export const DEFAULT_MAX_ITERATIONS = 110; export const DEFAULT_GRPC_PORT_START = 50051; // ============================================================================ -// Planner output action keys — used by HeadlessGoalExecutor to parse planner response -// These must match the strings the planner LLM outputs. +// Planner output action keys — the normalized `act` values AIAgent maps the +// planner's snake_case action_type strings onto (see FIXED_PROMPT_ACTIONS in +// AIAgent.ts); ActionExecutor routes its handler map on them and TestExecutor +// checks the completed/failed terminals. // ============================================================================ export const PLANNER_ACTION_TAP = 'tap'; export const PLANNER_ACTION_LONG_PRESS = 'longPress'; diff --git a/packages/common/src/env.ts b/packages/common/src/env.ts index c5defcc..e553c3d 100644 --- a/packages/common/src/env.ts +++ b/packages/common/src/env.ts @@ -5,8 +5,9 @@ import * as dotenv from 'dotenv'; import * as path from 'path'; import * as fs from 'fs'; // Backward-compatibility shim: these live in constants.ts, but consumers -// historically imported them through env.js — env.test.ts still does, and the -// constitution's Test Integrity rule forbids editing it to chase a new path. +// historically imported them through env.js and the re-export keeps that +// import path working — env.test.ts still imports parseModel and +// parseReasoningLevel via '../env.js'. export { MODEL_FORMAT_EXAMPLE, PROVIDER_ENV_VARS, diff --git a/packages/common/src/models/Hierarchy.ts b/packages/common/src/models/Hierarchy.ts index ef825c1..2dff15d 100644 --- a/packages/common/src/models/Hierarchy.ts +++ b/packages/common/src/models/Hierarchy.ts @@ -1,6 +1,7 @@ // Port of common/model/Hierarchy.dart — MINIMAL: parse + flatten + node properties -// The Dart file is ~108KB. We port only the subset used by FinalRunAgent and -// HeadlessActionExecutor for AI prompt building and grounding. +// We port only the subset the goal-executor (AIAgent, ActionExecutor, +// TestExecutor, GrounderResponseConverter) needs for AI prompt building +// and grounding. import { PLATFORM_ANDROID, PLATFORM_IOS } from '../constants.js'; diff --git a/packages/device-node/src/device/logWriteStream.ts b/packages/device-node/src/device/logWriteStream.ts index 22c0dda..051c428 100644 --- a/packages/device-node/src/device/logWriteStream.ts +++ b/packages/device-node/src/device/logWriteStream.ts @@ -86,9 +86,9 @@ export class LogWriteStreamRegistry { // The log call is guarded because THIS listener must not throw. `Logger.e` // is fallible INDEPENDENTLY of why this stream failed: the sink loop in - // `Logger._emit` (`packages/common/src/logger.ts:103-105`) runs each sink + // `Logger._emit` (`packages/common/src/logger.ts`) runs each sink // with no try/catch, and the CLI installs `ReportWriter.createLoggerSink()` - // (`packages/cli/src/reportWriter.ts:132`) — an unguarded synchronous + // (`packages/cli/src/reportWriter.ts`) — an unguarded synchronous // `fs.appendFileSync` to the runner log — so a full disk, a permissions // change or a removed artifacts directory makes the log call throw on its // own schedule. The two can also be one failure: this stream writes under