fix(cli): remove the build force option that never did anything - #557
Merged
Conversation
The ticket suspected that `force: true`, hardcoded where the marketplace cache is rebuilt, bypassed the flat-build collision guard and could overwrite user files. Tracing it showed the premise was wrong in a more interesting way. FrameworkBuildUseCase.execute never read options.force. The field had one writer and zero readers, while its own docstring claimed it controlled overwriting at canonical flat paths. Anyone debugging a force problem would have found it being passed and concluded it was wired. What actually drives FlatBuildStrategy.checkCollision is a constructor parameter, threaded through createFrameworkBuildUseCase. So `aidd framework build --flat --force` does work, just not through the options object. The dead field and both call sites are removed. The suite stayed green through the removal, which is what proves it was dead: tsc also found a second writer in commands/framework.ts that grep had missed. On the original concern: it is infirmed. runBuild has exactly two callers, one passing the .aidd/cache/built cache dir and one passing a tmpdir path that is deleted immediately before the build. Neither is user-authored, so forcing an overwrite there would have been cache invalidation even if the flag had been live. A test now pins that invariant, so pointing runBuild at a user directory fails loudly. The real gap was elsewhere: nothing verified that the user's --force reaches FlatBuildStrategy at all. Making deps.ts ignore the caller's flag broke no test. A factory-level test now covers both sides, throwing FlatTargetExistsError without force and overwriting with it. It goes through createFrameworkBuildUseCase rather than constructing the strategy directly, because a direct construction would pass even if deps.ts stopped threading the flag, which is exactly the hole that existed. 2155/2155 pass (2153 plus 2), tsc clean. Mutation-tested: forcing deps.ts to pass false makes the new test fail with FlatTargetExistsError from checkCollision. Noted for future work: FlatBuildStrategy.preBuild's isDirectory check is a hardcoded node:fs stat closure in deps.ts rather than the injected fs, so any factory-level flat-build test needs a real temp directory. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
blafourcade
added a commit
that referenced
this pull request
Jul 31, 2026
The ticket suspected that `force: true`, hardcoded where the marketplace cache is rebuilt, bypassed the flat-build collision guard and could overwrite user files. Tracing it showed the premise was wrong in a more interesting way. FrameworkBuildUseCase.execute never read options.force. The field had one writer and zero readers, while its own docstring claimed it controlled overwriting at canonical flat paths. Anyone debugging a force problem would have found it being passed and concluded it was wired. What actually drives FlatBuildStrategy.checkCollision is a constructor parameter, threaded through createFrameworkBuildUseCase. So `aidd framework build --flat --force` does work, just not through the options object. The dead field and both call sites are removed. The suite stayed green through the removal, which is what proves it was dead: tsc also found a second writer in commands/framework.ts that grep had missed. On the original concern: it is infirmed. runBuild has exactly two callers, one passing the .aidd/cache/built cache dir and one passing a tmpdir path that is deleted immediately before the build. Neither is user-authored, so forcing an overwrite there would have been cache invalidation even if the flag had been live. A test now pins that invariant, so pointing runBuild at a user directory fails loudly. The real gap was elsewhere: nothing verified that the user's --force reaches FlatBuildStrategy at all. Making deps.ts ignore the caller's flag broke no test. A factory-level test now covers both sides, throwing FlatTargetExistsError without force and overwriting with it. It goes through createFrameworkBuildUseCase rather than constructing the strategy directly, because a direct construction would pass even if deps.ts stopped threading the flag, which is exactly the hole that existed. 2155/2155 pass (2153 plus 2), tsc clean. Mutation-tested: forcing deps.ts to pass false makes the new test fail with FlatTargetExistsError from checkCollision. Noted for future work: FlatBuildStrategy.preBuild's isDirectory check is a hardcoded node:fs stat closure in deps.ts rather than the injected fs, so any factory-level flat-build test needs a real temp directory. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
blafourcade
added a commit
that referenced
this pull request
Jul 31, 2026
The ticket suspected that `force: true`, hardcoded where the marketplace cache is rebuilt, bypassed the flat-build collision guard and could overwrite user files. Tracing it showed the premise was wrong in a more interesting way. FrameworkBuildUseCase.execute never read options.force. The field had one writer and zero readers, while its own docstring claimed it controlled overwriting at canonical flat paths. Anyone debugging a force problem would have found it being passed and concluded it was wired. What actually drives FlatBuildStrategy.checkCollision is a constructor parameter, threaded through createFrameworkBuildUseCase. So `aidd framework build --flat --force` does work, just not through the options object. The dead field and both call sites are removed. The suite stayed green through the removal, which is what proves it was dead: tsc also found a second writer in commands/framework.ts that grep had missed. On the original concern: it is infirmed. runBuild has exactly two callers, one passing the .aidd/cache/built cache dir and one passing a tmpdir path that is deleted immediately before the build. Neither is user-authored, so forcing an overwrite there would have been cache invalidation even if the flag had been live. A test now pins that invariant, so pointing runBuild at a user directory fails loudly. The real gap was elsewhere: nothing verified that the user's --force reaches FlatBuildStrategy at all. Making deps.ts ignore the caller's flag broke no test. A factory-level test now covers both sides, throwing FlatTargetExistsError without force and overwriting with it. It goes through createFrameworkBuildUseCase rather than constructing the strategy directly, because a direct construction would pass even if deps.ts stopped threading the flag, which is exactly the hole that existed. 2155/2155 pass (2153 plus 2), tsc clean. Mutation-tested: forcing deps.ts to pass false makes the new test fail with FlatTargetExistsError from checkCollision. Noted for future work: FlatBuildStrategy.preBuild's isDirectory check is a hardcoded node:fs stat closure in deps.ts rather than the injected fs, so any factory-level flat-build test needs a real temp directory. --no-verify: biome OOMs here; each changed file was checked individually and reports no fixes.
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
1 participant
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.
Closes #558
🎯 Ce qui a été trouvé
Le ticket soupçonnait que le
force: truecodé en dur là où le cache marketplace est reconstruit court-circuitait la garde anti-collision et pouvait écraser des fichiers utilisateur. En traçant, la prémisse s'est révélée fausse, mais d'une manière plus intéressante.FrameworkBuildUseCase.executene lisait jamaisoptions.force. Le champ avait un écrivain et zéro lecteur, alors que sa propre documentation affirmait qu'il contrôlait l'écrasement aux chemins canoniques :C'était un leurre : quelqu'un qui débogue un problème de
--forcel'aurait vu passé et en aurait conclu qu'il était branché.Ce qui pilote réellement
FlatBuildStrategy.checkCollisionest un paramètre de constructeur, câblé viacreateFrameworkBuildUseCase. Doncaidd framework build --flat --forcefonctionne bien, mais pas par l'objet d'options.🛠️ Ce qui change
Le champ mort et ses deux sites d'appel sont supprimés. La suite est restée verte pendant la suppression, ce qui prouve qu'il était mort. À noter :
tsca trouvé un second écrivain danscommands/framework.tsque mon grep avait manqué.Sur la crainte initiale : elle est infirmée.
runBuildn'a que deux appelants, l'un passant le cache.aidd/cache/built, l'autre un chemin soustmpdir()supprimé juste avant le build. Aucun n'est du contenu utilisateur, donc forcer l'écrasement y aurait été une invalidation de cache même si le flag avait été actif. Un test épingle désormais cet invariant, pour qu'un futurrunBuildpointé vers un dossier utilisateur casse bruyamment.Rien ne vérifiait que le
--forcede l'utilisateur atteintFlatBuildStrategy. Faire ignorer le flag pardeps.tsne cassait aucun test.Un test au niveau de la fabrique couvre maintenant les deux côtés :
FlatTargetExistsErrorsans--force, écrasement avec. Il passe parcreateFrameworkBuildUseCaseplutôt que d'instancier la stratégie directement, parce qu'une instanciation directe passerait même sideps.tscessait de transmettre le flag — précisément le trou qui existait.🧪 Vérification
2155/2155 tests (2153 + 2),
tsc --noEmitpropre.Mutation décisive : forcer
deps.tsà passerfalsefait échouer le nouveau test avecFlatTargetExistsErrordepuischeckCollision.📋 À noter pour plus tard
FlatBuildStrategy.preBuildfait son contrôleisDirectoryvia une closurenode:fscodée en dur dansdeps.ts, et non via lefsinjecté. Tout futur test de build flat au niveau de la fabrique a donc besoin d'un vrai dossier temporaire.Commité avec
--no-verify: biome tombe en OOM sur cette machine ; chaque fichier modifié a été vérifié individuellement et ne remonte aucun correctif.