Both findings from my last review have been addressed cleanly. The double-space in formatSource's default branch is gone — it's a single space now. And the comment at the av.Source.Kind/av.Source.Value assignment in createVersion is exactly what I asked for: a crisp one-liner explaining why unconditionally overwriting those fields after SourceFromGitInfo is safe.
The rest of the change is solid. The sourceFromBuildStack helper centralises the kind/value mapping, DetectedStack is now properly assigned on the resolution struct before sourceFromBuildStack reads it, and the new TestBuildSaga_AutoStackRecordsDetectedSource test covers that path end-to-end alongside the existing dockerfile and image tests. The blackbox test in image_source_test.go gives real confidence that the full roundtrip — deploy, persist, read back via app status --format json — produces the expected shape. The backward-compat shim in analysisSource remains a thoughtful touch.
I'm satisfied. This is ready for human review.
Posted to the PR's comment threads when you submit.
source.go line 25 is now "%s %s" (single space). This thread is done.// SourceFromGitInfo only sets git provenance; the build source owns Kind and Value. A future reader now has what they need without having to trace into a second file. Happy to close this out.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1106,
"verdict": "ready",
"event": "comment",
"summary": "Both findings from my last review have been addressed cleanly. The double-space in `formatSource`'s `default` branch is gone — it's a single space now. And the comment at the `av.Source.Kind`/`av.Source.Value` assignment in `createVersion` is exactly what I asked for: a crisp one-liner explaining why unconditionally overwriting those fields after `SourceFromGitInfo` is safe.\n\nThe rest of the change is solid. The `sourceFromBuildStack` helper centralises the kind/value mapping, `DetectedStack` is now properly assigned on the resolution struct before `sourceFromBuildStack` reads it, and the new `TestBuildSaga_AutoStackRecordsDetectedSource` test covers that path end-to-end alongside the existing dockerfile and image tests. The blackbox test in `image_source_test.go` gives real confidence that the full roundtrip — deploy, persist, read back via `app status --format json` — produces the expected shape. The backward-compat shim in `analysisSource` remains a thoughtful touch.\n\nI'm satisfied. This is ready for human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6dyYQJ",
"body": "Confirmed fixed — the format string in `source.go` line 25 is now `\"%s %s\"` (single space). This thread is done.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6dyYQR",
"body": "The comment landed exactly as I had in mind: `// SourceFromGitInfo only sets git provenance; the build source owns Kind and Value.` A future reader now has what they need without having to trace into a second file. Happy to close this out.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"draft": true
}This PR threads a new source.kind / source.value pair all the way from build detection through the entity store, the app-status API, and the CLI. The design is clean and the test coverage is solid: there are unit tests for formatSource, analysisSource, the saga pipeline (dockerfile, image, auto-stack paths), the app server's GetConfiguration, and a blackbox test that asserts the correct JSON shape after a real image deploy. The backward-compatibility shim in analysisSource (falling back to the old stack field when a new CLI talks to an old server) is a thoughtful touch.
Two findings worth calling out before this merges:
1. Double space in the default branch of formatSource (minor, but user-visible)
cli/commands/source.go line 25 has:
return fmt.Sprintf("%s %s", kind, value)
That's two spaces between kind and value. The default arm handles future/unknown source kinds, so it's unlikely to fire in practice right now, but it's still a silent formatting bug. A single space is almost certainly intended.
2. createVersion overwrites Source.Kind/Value after SourceFromGitInfo — order matters
In build_saga.go around lines 477–489, the function first calls deploylifecycle.SourceFromGitInfo(gitInfo) and assigns the result to av.Source, then immediately overwrites av.Source.Kind and av.Source.Value. The intent — keeping git provenance fields while layering the source kind on top — is correct. But the assignment order creates a subtle invariant: if in.SourceKind is ever empty (e.g. for an unknown stack that falls through sourceFromBuildStack), it silently clears whatever kind SourceFromGitInfo might have set. This is probably fine today since SourceFromGitInfo only sets git fields (branch, sha, repository), not kind/value. Worth a brief comment at the assignment site so future readers don't have to trace through two files to confirm this is safe.
The draft flag is noted; as a draft I'm reviewing whether it's ready for human review. Aside from the cosmetic double-space fix, the logic is sound and the tests have real teeth — I'd consider it close to merge-ready pending those two observations.
%s and %s — the default case renders kind value with two spaces. Almost certainly a typo; should be a single space.av.Source.Kind and av.Source.Value after SourceFromGitInfo has already populated av.Source. That's intentional — SourceFromGitInfo only touches git fields — but a reader tracing through two files to verify this is a genuine trap. A one-line comment here would pay for itself quickly.Verdict: caveats