Skip to content

chore: add agent-efficiency regression guards#1174

Merged
thymikee merged 13 commits into
mainfrom
devin/1783624588-agent-efficiency-ratchets
Jul 10, 2026
Merged

chore: add agent-efficiency regression guards#1174
thymikee merged 13 commits into
mainfrom
devin/1783624588-agent-efficiency-ratchets

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a single-pass layering model behind pnpm check:layering: R1–R3 share resolved import edges with a zero value-import-cycle guard and an exact-identity, down-only back-edge baseline. CI compares the committed baseline to the PR base, so removals pass while additions and same-count edge replacements fail.
  • Adds deterministic output-economy coverage for agent-facing payloads. Byte/line budgets are monotonic against the merge-base baseline (or the branch's first introduced baseline), while exact reviewed waivers document meaningful semantic increases. Regression coverage proves a same-PR budget increase fails.
  • Measures a representative MCP snapshot result as the complete model-visible payload: optimized text plus structured content. Independent actionability floors protect refs, generation pins, settle tails, recovery warnings, retryability, and error hints; SkillGym/help-conformance and perf remain the outcome/live-signal layers.
  • Adds pnpm explain:command <name|catalogKey> [--json] [--full], derived from typed command, CLI, capability, timeout, and dispatch declarations. CommandDescriptor.ownerFiles is required; DAEMON_ROUTE_HANDLERS binds each route's owner file, production loader, and dispatch adapter in one declaration without changing fallback bundling.
  • Keeps default explanations compact and command-specific; exhaustive flags remain available through --full/--json. CLI alias implied flags are restricted to boolean keyof CliFlags keys.
  • Tracks stripping development-only owner metadata from runtime bundles without recreating a parallel registry in Keep command owner metadata tooling-only without duplicating registries #1178.

Validation

  • pnpm build
  • pnpm format:check
  • pnpm typecheck
  • pnpm lint
  • pnpm check:layering
  • node --experimental-strip-types scripts/layering/check.ts --base origin/main
  • OUTPUT_ECONOMY_BASE=$(git merge-base HEAD origin/main) pnpm test:output-economy
  • pnpm test:unit
  • pnpm check:fallow --base origin/main

Link to Devin session: https://app.devin.ai/sessions/b1c3093bf2b248ca82a1a5df89152d02
Requested by: @thymikee

@thymikee thymikee self-assigned this Jul 9, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB +4.3 kB
JS gzip 517.3 kB 517.9 kB +611 B
npm tarball 623.4 kB 624.2 kB +750 B
npm unpacked 2.2 MB 2.2 MB +4.7 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 19.0 ms 21.9 ms +3.0 ms
CLI --help 36.7 ms 41.4 ms +4.7 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/sdk-batch.js +3.7 kB +283 B
dist/src/cli-help.js 0 B +138 B
dist/src/internal/daemon.js +465 B +136 B
dist/src/cli.js +221 B +54 B

@github-actions

github-actions Bot commented Jul 9, 2026

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

@thymikee

thymikee commented Jul 9, 2026

Copy link
Copy Markdown
Member

Review found three blockers before ready-for-human:

  1. The back-edge ratchet is not actually down-only in CI. check.ts reads the baseline from the PR head, and --update-baseline rewrites it unconditionally. A PR can add a back-edge, increment back-edge-baseline.json, and pass. Pair-level counts also allow replacing one existing edge with a different edge in the same pair without detection. Please compare baseline evolution against the merge base and/or pin exact edge identities, then add regression tests for both baseline growth and same-count edge replacement.

  2. explain:command guesses owners and omits real production paths. Reproducing apps --json omits src/commands/management/app.ts. Generic-dispatch commands such as screenshot omit src/core/dispatch.ts; camelCase routes such as recordTrace and reactNative do not map to record-trace.ts / react-native.ts. Derive ownership from an existing registry/declaration rather than filename conventions, and cover representative family, generic-dispatch, and mapped-route commands.

  3. The default explanation is not compact. apps emits 36 infrastructure/provider flags plus global flags in its normal text output. That conflicts with this PRs progressive, agent-efficient output goal. Keep the default focused on command-specific flags and the key routing/ownership facts; expose exhaustive flags through --json or an explicit full mode.

Local validation: the layering model/check, output-economy project, and focused command-explain tests all pass. All 22 GitHub checks are green, but the current tests do not exercise the failure modes above.

Please address all actionable points and reply with what changed, what was validated, and anything still blocked.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Blocker 1 fixed in b0a23dc: back-edge-baseline.json now stores exact edge identities, R5 detects added/removed identities, and R6 compares the committed baseline against --base/merge-base so additions and same-count replacements fail while removals pass. Validated with pnpm check:layering, node --experimental-strip-types scripts/layering/check.ts --base origin/main, and the new model.test.ts replacement/addition/removal cases.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Blocker 2 fixed in b0a23dc: explain:command no longer guesses per-command owner filenames; it reads declared command owner files, adds generic-dispatch ownership via src/core/dispatch.ts, and keeps actual daemon-route owners like src/daemon/handlers/record-trace.ts. Coverage now asserts apps -> src/commands/management/app.ts, screenshot -> src/commands/capture/screenshot.ts + src/core/dispatch.ts, and record/trace route owners.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Blocker 3 fixed in b0a23dc: default explain:command text now shows only command-specific flags, while --full and --json expose exhaustive supported/global flags. The regression test exercises apps as the compactness case (flags: --all, no default supported:/global:, exhaustive arrays preserved in JSON).

@thymikee

thymikee commented Jul 9, 2026

Copy link
Copy Markdown
Member

Follow-up architecture blocker from the fix pass:

The owner-path correction now adds two new parallel registries: COMMAND_OWNER_FILES in src/commands/command-owner-files.ts and DAEMON_ROUTE_OWNER_FILES in src/commands/command-explain.ts:299. This recreates the command/route sets away from their owning declarations, contrary to ADR 0008 and the repo rule that machine-readable claims live with the code they describe. The new tests prove selected examples and file existence, but they do not prove that a command has its real owner: an omitted command-map entry still returns registry/family files, and an incorrect route mapping can point to an unrelated existing file while staying green.

Please colocate command owner metadata with the command facet/descriptor and route owner metadata with the daemon route/handler declaration, then derive explain:command from those typed sources. Completeness should fail when a new explainable command or route lacks owner metadata.

One smaller type-safety regression came with the alias extraction: CliCommandAlias.impliedFlags is readonly string[], and args.ts:121-122 writes those strings through Record<string, unknown>. A typo now compiles and silently creates a non-flag property. Type implied flags as FlagKey / keyof CliFlags and keep application type-checked.

The original three behavioral blockers now reproduce as fixed, and focused local validation passes. This follow-up is about preserving the registry architecture rather than replacing one drift source with another.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Addressed the follow-up architecture blocker in cc5d4563a: command owners are required on each CommandDescriptor; daemon route names derive from a typed route declaration carrying each handler owner; and explain:command reads both colocated sources directly with completeness/file-existence coverage. Alias implied flags are restricted to boolean keyof CliFlags keys and applied without Record<string, unknown>. Full requested validation passes.

thymikee and others added 10 commits July 10, 2026 07:09
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…src files

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…cy-derived error, like-for-like screenshot surfaces

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…owners

Surface true CLI aliases from parser normalization (long-press, metrics,
tap, launch, relaunch) distinct from catalog keys, preserving implied-flag
semantics (relaunch => open --relaunch). Extract the canonical single-line
usage builder to src/utils/cli-usage.ts so schemas without usageOverride
include positionals and flags. Replace guessed handler paths with a
completeness-checked daemon-route owner map keyed by the closed
DaemonCommandRoute union, fixing silently-dropped non-kebab routes
(reactNative, recordTrace) and generic dispatch. Add table-driven coverage
for aliases, synthesized usage, split-family/route-variant/dispatch owners,
structured output, and explain:command CLI exit/stdout/stderr.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration devin-ai-integration Bot force-pushed the devin/1783624588-agent-efficiency-ratchets branch from cc5d456 to 7d2f681 Compare July 10, 2026 07:10
@thymikee

Copy link
Copy Markdown
Member

One architecture blocker remains at 7d2f68158 before ready-for-human.

Command owner metadata and alias implied-flag typing are now fixed. Daemon route ownership is still a parallel claim, though: DAEMON_COMMAND_ROUTES in src/core/command-descriptor/daemon-routes.ts contains only owner-file strings, while the real route-to-handler declarations remain the separate lazy imports and switch in src/daemon/request-handler-chain.ts. The new test only checks that each owner path exists. A route can therefore name an unrelated existing file while production dispatch continues to another handler, and every new completeness/existence test stays green - the same drift mode called out in the previous review.

Please colocate each route's owner metadata with the actual loader/dispatch declaration, or add a structural derivation that makes the handler used by runRequestHandlerChain and the owner shown by explain:command one declaration rather than two manually synchronized tables. Add a regression test that fails when a route's explain owner diverges from its production handler, not only when the path is missing.

The rest of the requested fixes reproduce as resolved, and all 22 checks pass.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Addressed in 2bd4ca6b9: DAEMON_ROUTE_HANDLERS now binds each owner path to the exact lazy loader and dispatch adapter used by runRequestHandlerChain; generic fallback dispatch also loads through that same entry. The parallel core route-owner registry is deleted, and regressions now fail if owner/import declarations diverge or explain:command stops consuming the production route projection.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration devin-ai-integration Bot changed the title chore: add agent-efficiency architecture ratchets chore: add agent-efficiency regression guards Jul 10, 2026
@thymikee

Copy link
Copy Markdown
Member

Final re-review of c491def20: no actionable blockers remain.

The route-owner drift is now closed structurally: each production route declaration binds its owner path, exact loader, and dispatch adapter in DAEMON_ROUTE_HANDLERS; runRequestHandlerChain and explain:command consume that same projection, the old parallel core registry is gone, and the regression test checks owner/import agreement. Generic dispatch retains its prior bundling behavior through the shared loader.

The added output-budget ratchet also holds: CI fetches the target history, compares candidate bytes/lines against the target-base (or first introduced baseline), rejects baseline inflation and removed measured surfaces, and permits an increase only through an exact reviewed metric plus reason. The production MCP fixture measures optimized text and structured content together. All 22 checks pass.

This is tooling/contract work; no device evidence is required. Issue #1178 correctly tracks the non-blocking follow-up to erase owner metadata from production bundles without recreating a registry. Ready for maintainer judgment.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant