Repository navigation
Conversation
ChrisonSimtian
left a comment
There was a problem hiding this comment.
a lot of this code is static methods and static classes. I think this one needs a bit more hands on from a human, the code quality unfortunately trips AI to think all-static is a good and preferred pattern.
might be worth to nudge your agent into the right direction and have it have another go 😊
662c6f8 to
46c4456
Compare
Addresses the review: the introspection code was a static class over ambient state, which the surrounding statics made easy to reach for but which cost correctness and coverage. BuildIntrospectionService is now a sealed class holding the resolved request. BuildManager builds one with `For(build)` and reuses it at the gate, on the failure path and before the outcome tables. Those three sites each used to re-read the flags out of ParameterService, so a single run answered "is this introspection?" three separate times from process-global state that nothing kept in agreement. The invoked targets are read once now too, instead of once for the planner and again inside GetDocument. That also removes a test-only overload, and with it a real gap. Production described a build through GetJsonString(build, targets), which projects the class-level [Requires<T>] requirements; the specs called an overload taking an explicit version, and that one passed no requirements at all. The specs were asserting a document the product never emits, and the projection was uncovered. BuildGraphUtility now takes the version on the same path production uses, and a new spec pins a build-level requirement reaching the document. The argument-based IsRequested stays static: the CLI asks it before a build exists, and it is a pure function of its arguments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VyD69qYha8hBMuja7opyj2
|
Fair call, and it turned out to be hiding a real bug rather than just style. Pushed in e6c358d.
The part worth flagging: dropping the static shape removed a test-only overload, and that overload was papering over a coverage gap. Production described a build through One thing I deliberately left static: I did not touch Full suite green: 849 passing. |
develop ran a formatting pass over src/Fallout.Build/Execution. Four files conflicted, and on develop's side every change was style only: braces around single-statement blocks, blank lines between statements, re-indented array initializers and a moved using. Kept this branch's logic in all four and applied develop's style around it: - BuildManager.cs — Finish() keeps the introspection guard, now braced. - Extensions/HandleHelpRequestsAttribute.cs — keeps the projected model, the GetBufferWidth() fallback and the parameter tuple; braces and blank lines follow develop. - Extensions/SerializeBuildGraphAttribute.cs — kept both usings and develop's unqualified Log.Verbose. - ValueInjectionUtility.cs — kept the Describe and Json entries inside develop's re-indented array. Also braced the `if (!projected.Listed)` guard in GetTargetsText. It sits just outside the conflict, and leaving it unbraced would have made that one method the only unformatted part of the file. Verified: dotnet build fallout.slnx succeeds with 0 errors, and dotnet test fallout.slnx passes with 0 failures and no Verify snapshot drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnT6hhAWLHoiVdx8NMAu7u
Conflict with
|
| File | What was kept |
|---|---|
BuildManager.cs |
Finish() keeps the introspection is { IsRequestedForRun: true } guard, now braced. |
Extensions/HandleHelpRequestsAttribute.cs |
The projected model, the GetBufferWidth() fallback and the (Member, Model) tuple. Braces and blank lines follow develop. |
Extensions/SerializeBuildGraphAttribute.cs |
Both usings, plus develop's unqualified Log.Verbose. |
ValueInjectionUtility.cs |
The Describe and Json entries, inside develop's re-indented array. |
One change sits just outside a conflict hunk: the if (!projected.Listed) guard in GetTargetsText is now braced. Leaving it alone would have made that guard the only unformatted statement in a file develop had just formatted end to end.
Verified after merging:
dotnet build fallout.slnx— 0 errors.dotnet test fallout.slnx— 0 failures across all 13 test projects, includingBuildGraphUtilitySpecs,BuildIntrospectionServiceSpecsandHandleHelpRequestsSpecs. No Verify snapshot drift, so no.received.file was produced../build.sh --helprenders--describeand--jsonthrough the mergedHandleHelpRequestsAttribute, which was the largest conflict../build.sh --describeemits a well-formed document (19 targets, 22 parameters) and./build.sh --plan --jsonemitsversion/invokedTargets/plan. Both exercise theBuildManagershort-circuit and theFinish()guard that were resolved by hand..fallout/build.schema.jsondoes not drift: booting the build project leavesgit statusclean.
The PR is mergeable again.
🤖 Generated with Claude Code
ChrisonSimtian
left a comment
There was a problem hiding this comment.
@phmatray Sorry for the long wait. You answered my review on 3 Sep and merged develop on 9 Sep, and we did not reply. That was our fault, and I am sorry. Thank you for your patience and for the careful work here.
My static-code concern is mostly resolved. BuildIntrospectionService is an instance class now, and the gap you found in the specs was a good catch.
What I checked
I built this branch and ran --describe and --plan --json against this repo's own build.
- Standard output is only JSON. Standard error is empty. The working tree is unchanged afterwards.
- A skipped dependency reports
"via parameter". An invoked target is never reported as skipped. This matches the executor. - An unknown target gives a JSON error and exit code 255.
[Secret]parameters are flagged and carry no value.--helpdiffers fromdeveloponly by the two new parameters.Build.Specs(277) andCli.Specs(47) pass. The branch merges cleanly into currentdevelop.
Changes needed
1. The acceptance criteria of #642 are not fully met.
defaultandallowedValuesarenullfor all 22 parameters. #642 asks for the default value. Please fill them in, or remove the fields until they carry data. A consumer cannot tell "no default" from "not reported".allowedValuesmatters most forconfiguration,verbosityandhost.typeshows internal .NET types.--targetand--skipreportIReadOnlyCollection<ExecutableTarget>, which users never type. Renaming an internal type would then break the JSON contract. Please map to a small stable set (for examplestring,bool,int,path,string[],enum).- #642 asks for a machine-readable error when the build project fails to load. A compile error in the build project gives no JSON, only text on stderr. I read this in the code and did not run it. Please cover it, or say in the description that it is out of scope.
2. The script changes affect every run, not only introspection.
build.ps1,build.shand the templates now write the banner andtool restoreoutput to stderr for all users. Some CI systems treat stderr output as a failure signal.- In
build.ps1,2>&1with$ErrorActionPreference = "Stop"may fail on Windows PowerShell 5.1 whendotnet tool restorewrites to stderr. CI only ran on ubuntu, and I could not test Windows. - Please redirect only when the arguments request introspection. Or show a passing run on Windows PowerShell 5.1.
3. The riskiest code has no tests.
- Nothing tests the gate in
BuildManager: no extension runs, theFinish()guard, the error path and the exit codes. Nothing tests the stdout redirect inRunCommandeither. I checked the gate by hand, but nothing would catch a regression. - None of the new or changed specs use
// Arrange,// Act,// Assert(CONTRIBUTING.md, "Pull requests").
4. Public API and PR housekeeping.
- The PR adds public
FalloutBuild.DescribeandFalloutBuild.Json, andPlan { internal set; }. A consumer's ownJsonorDescribemember on their build class could collide. I have not tested that. Please follow "Proposing new public API" in CONTRIBUTING.md, and say whether[Experimental]fits. - The PR needs labels (
target/vCurrentand a changelog category) and a milestone. I will add these. - Please add docs for the two flags and the JSON documents, including the schema version rules.
- Please curate the 11 commits. Fold "Scaffold draft PR", "Address code-review findings" and the merge commit into the commits they belong to. Consider moving the
--helpcrash fix for #616 into its own commit.
Smaller points (not blocking)
BuildGraphUtilityhas fourGetModeland fourGetJsonStringoverloads, and the specs use the shorter ones. Please collapse them.- The skip logic is copied from
BuildExecutor, and only the reason string is shared. Please share the predicate so the two cannot drift.IsRequestedalso reimplements the flag parsing. - The error
kindis the .NET type name, which is just"Exception"for an unknown target. A stable error code would help machines. #643 covers exit codes. - Please shorten the comments and use plain, literal English, as in AGENTS.md rule 8 and the
plain-englishskill.
If anything is unclear, ask here and I will answer the same day. Once these are done I will review again promptly.
dennisdoomen
left a comment
There was a problem hiding this comment.
🤔 I'm struggling with this PR. It clearly was built by an AI and because of that, resulted in something that is hacked into the existing codebase and therefor very hard to follow.
🔧 Lots of new code has the wrong namespace.
🔧 The new tests don't follow the AAA principle and are optimized to be as small as possible instead of demonstrating the arrange, act and assert parts
| Write-Output "PowerShell $($PSVersionTable.PSEdition) version $($PSVersionTable.PSVersion)" | ||
| # Provisioning chatter goes to standard error, so standard output carries only what the build | ||
| # itself writes. That is what lets `./build.ps1 --describe | ConvertFrom-Json` work. | ||
| [Console]::Error.WriteLine("PowerShell $($PSVersionTable.PSEdition) version $($PSVersionTable.PSVersion)") |
There was a problem hiding this comment.
What about consumers that didn't update their build.ps1?
| using System.Linq; | ||
| using Fallout.Build.Execution.Extensions; | ||
| using Fallout.Common.Utilities; | ||
| using Fallout.Common.ValueInjection; |
| internal const int ErrorSchemaVersion = 1; | ||
|
|
||
| private readonly bool describe; | ||
| private readonly bool planAsJson; |
| /// different shapes, and one number could not tell a consumer which contract it received — | ||
| /// nor could it be bumped for one without falsely signalling a break in the other. | ||
| /// </summary> | ||
| internal const int PlanSchemaVersion = 1; |
There was a problem hiding this comment.
🔧 Doesn't need to be internal. Same with the others.
| // nothing" structural: IOnBuildCreated alone rewrites .fallout/build.schema.json | ||
| // (HandleShellCompletionAttribute) and can block on Console.ReadKey | ||
| // (UpdateNotificationAttribute), which would deadlock a piped consumer. | ||
| if (introspection.IsRequestedForRun) |
There was a problem hiding this comment.
🔧🤖 Execute grows by ~20 more lines; extract the introspection gate into its own method.
| /// file I/O) so the contract can be snapshot-tested without driving a build. | ||
| /// </para> | ||
| /// </summary> | ||
| internal static class BuildGraphUtility |
There was a problem hiding this comment.
🤔 This class seems to be misused and suddenly has a ton of internal members which I do not understand why they are needed.
| /// the property here would make every request look like an ordinary build. | ||
| /// </remarks> | ||
| // --plan alone keeps its existing meaning (the HTML graph); only --json redirects it here. | ||
| internal static BuildIntrospectionService For(FalloutBuild build) |
There was a problem hiding this comment.
🔧Make the public API of this class public and avoid internal. Don't touch internals through tests only. Always use the public API.
| var skipped = (skippedTargets ?? new string[0]) | ||
| .Select(x => x.Replace("-", string.Empty)).ToList(); | ||
|
|
||
| var entries = plan |
There was a problem hiding this comment.
🔧 Needs a rewrite as it is unreadable for a human being
| using Fallout.Common.Utilities; | ||
| using Fallout.Common.ValueInjection; | ||
|
|
||
| namespace Fallout.Common.Execution; |
| // A read-only introspection request (--describe, --plan --json) promises a JSON document on | ||
| // standard output. Compiling the build project writes there too, so its output would land | ||
| // inside the document a consumer is parsing — send it to standard error for those runs. | ||
| var quietBuildStep = BuildIntrospectionService.IsRequested(forwardedArgs); |
There was a problem hiding this comment.
⛏ IsRequested is a bit of a weird way to ask the service
Implements #642.
Closes #642.
Executing the implementation plan task-by-task; the checklist below — and the plan on the issue — are ticked as each task lands. Opened as a draft — will be marked ready after the final task and a code-review pass.
Two constraints shaped the design, both verified in the tree:
BuildGraphUtilityalready projects targets and all four relation kinds into a schema-versioned document, so this extends that model additively rather than inventing a second one.SchemaVersionstays1, which also delivers Emit declared build parameters into build-graph.json #499'sparameters[].BuildManager.ExecuterunsToolRequirementService.EnsureToolRequirements(writesnuget.csproj, shells out todotnet restore) before theIOnBuildInitializedhooks. So the "runs no external tool" criterion rules out another extension; the emission short-circuits above that line.The commands land as the build-side parameters
--describeand--plan --json, becauseCommandDispatcherroutes bare arguments to the build and reserves:-prefixed tokens for CLI commands.Plan
--describeemits the model before tool requirements run--plan --jsonemits the resolved execution plan--helpfrom the shared model--helpis byte-identical tomainVerified by building
mainin a scratch worktree and diffing the output: the only difference is the version string.--helptakes its displayed fields from the shared model (so the two views cannot drift) but keeps ordering — of targets, of each dependency line, and of parameters — from the declarations, because declaration order carries the pipeline reading a human wants.Code review
A review of the branch surfaced 15 findings; all are addressed. The ones worth calling out:
BuildExecutor.MarkTargetSkippedonly skips when!target.Invoked, so--skipnever stops an explicitly invoked target. The projection had no such guard and reported"skip": "via parameter"for it — a consumer driving CI off the plan would conclude nothing runs. Fixed, with a spec that pins the parity.IOnBuildCreatedextensions, so each misbehaving extension needed its own opt-out — andUpdateNotificationAttributecallsConsole.ReadKey(), which would have deadlocked a piped--describe. The gate now fires before any extension runs, and the per-extension opt-out is gone.IsRequestedreads each flag from the injected property or the raw arguments, becauseInjectParameterValuesAttributeis itself anIOnBuildCreatedextension.[Requires<T>]was invisible. That attribute targets Class/Interface only, so class-level requirements could never appear on a target and were missing entirely. They now project to a root-leveltoolRequirements[].Type.FullNameleaked the runtime version into the contract for constructed generics (List\1[[System.String, …, Version=10.0.0.0, …]]`). Generic arguments are now rendered recursively.listfield that could never be false, gave the plan and error documents their own schema-version constants, and de-duplicated the serializer options, version lookup, and skip-reason string that had been copy-pasted.Making standard output actually parseable
Running the feature revealed that the document was correct but unreachable through any shipped entry point — everything upstream of the build wrote to stdout too, so
--describecould not be piped into a parser. Three fixes:Fallout.Clicompiled the build project with its output inherited on stdout. For an introspection request that step now goes to standard error. Measured through the locally built CLI: 12 123 bytes of JSON on stdout, 8 582 bytes of build noise on stderr. Ordinary runs are unchanged — build output stays on stdout.build.ps1/build.sh(and the shipped templates, kept in sync) printed their banner anddotnet tool restoreoutput to stdout. That is provisioning diagnostics, so it moves to standard error.--helpno longer aborts when stdout has no console.Console.BufferWidththrowsIOExceptionbehind a pipe or on a console-less agent, which killed--helpafter the target list and before the parameters. The wrap width is cosmetic, so it now falls back to the existing 90-column cap. This closes--helpcrashes with IOException when stdout has no console handle #616.Also: the emitted documents are newline-terminated, matching
SchemaUtility's JSON../build.ps1 --describestays unparseable until a Fallout release carrying the CLI fix is installed, because the script delegates to whateverdotnet falloutis on the machine. Nothing in this PR can change an already-installed tool.Follow-ups
[Requires]resolves its version over the network insideNuGetPackageRequirement's constructor, before any short-circuit. Pre-existing, affects--helptoo; "runs no external tool" is scoped to this code path.IOnBuildPlannedextension hook this deliberately avoided is worth revisiting once FT-7 ([Foundation] FT-7: Formalize the build extension pipeline (deterministic ordering + typed phases) #312) formalizes the extension pipeline; Declare target effects and gate them at plan time #644 would use it too.🤖 Generated with Claude Code