Conversation
Add manifest compilation (.core/manifest.yaml → core.json) with build metadata (commit, tag, timestamp, signature) and marketplace index generation by crawling directories for compiled or source manifests. New files: - manifest/compile.go: CompiledManifest, Compile(), ParseCompiled(), WriteCompiled(), LoadCompiled(), MarshalJSON() - marketplace/builder.go: Builder.BuildFromDirs(), BuildFromManifests(), WriteIndex() - cmd/scm/: CLI commands — compile, index, export Tests: 26 new (12 manifest, 14 marketplace), all passing. Co-Authored-By: Virgil <virgil@lethean.io>
Add manifest compilation (.core/manifest.yaml → core.json) with build metadata (commit, tag, timestamp, signature) and marketplace index generation by crawling directories for compiled or source manifests. New files: - manifest/compile.go: CompiledManifest, Compile(), ParseCompiled(), WriteCompiled(), LoadCompiled(), MarshalJSON() - marketplace/builder.go: Builder.BuildFromDirs(), BuildFromManifests(), WriteIndex() - cmd/scm/: CLI commands — compile, index, export Tests: 26 new (12 manifest, 14 marketplace), all passing. Co-Authored-By: Virgil <virgil@lethean.io>
…aller NewInstaller now takes an io.Medium parameter for filesystem operations, replacing direct os.MkdirAll and os.RemoveAll calls. Also fixes installer tests to use correct manifest path (.core/manifest.yaml) and force-add .core/ directory past .gitignore. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
…ll with go-io Medium Replace all os.ReadFile, os.WriteFile, and os.MkdirAll calls in marketplace/discovery.go, marketplace/builder.go, and jobrunner/journal.go with coreio.Local equivalents (Read, Write, EnsureDir). Co-Authored-By: Virgil <virgil@lethean.io>
Replace all remaining fmt.Errorf and errors.New calls in production
code with coreerr.E("caller.Method", "message", err) from go-log.
This standardises error handling across 23 files using the structured
error convention already established in the plugin package.
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
33 translation keys for collect, forge, gitea commands. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request standardises error handling across the codebase by replacing Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
forge/labels.go (1)
80-91: Consider distinguishing "not found" from API errors.
EnsureLabelinterprets any error fromGetLabelByNameas "label not found" and attempts creation. IfListRepoLabelsfails due to a network or permission error, this logic will still attempt to create the label, potentially masking the original failure.A sentinel error (e.g.,
ErrLabelNotFound) or a typed error could allowEnsureLabelto differentiate between "not found" and genuine failures. This is pre-existing behaviour, so flagging as optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@forge/labels.go` around lines 80 - 91, GetLabelByName currently returns any error and EnsureLabel treats them as "not found"; change this so GetLabelByName (or the underlying ListRepoLabels) returns a sentinel error (e.g., var ErrLabelNotFound = errors.New("label not found")) or a typed error when the label truly does not exist, then update EnsureLabel to only call CreateRepoLabel(owner, repo, ...) when err == ErrLabelNotFound (or the error type matches) and to return the original error for all other failures; update any callers/tests accordingly and keep the unique symbols GetLabelByName, EnsureLabel, ListRepoLabels, CreateRepoLabel in mind while making the change.jobrunner/handlers/tick_parent.go (1)
11-11: Import alias may cause confusion.The package path
forge.lthn.ai/core/go-logsuggests a logging package, but it's aliased ascoreerrfor error handling. This naming mismatch could confuse maintainers. Consider verifying this is the intended package or adding a brief comment explaining the dual-purpose nature of the package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobrunner/handlers/tick_parent.go` at line 11, The import alias coreerr in tick_parent.go is misleading for the package path forge.lthn.ai/core/go-log; verify the intended package and either rename the alias to a clearer identifier (e.g., corelog or logpkg) and update all usages of coreerr in this file (functions/methods that call coreerr.*), or add a succinct inline comment next to the import explaining that this logging package is used for error reporting; ensure the chosen alias matches other files for consistency.repos/registry.go (1)
105-150: Minor inconsistency in error wrapping.The errors from
os.Getwd()(line 108) andos.UserHomeDir()(line 133) are returned directly withoutcoreerr.Ewrapping, whilst other error paths in this file use structured error wrapping. Consider wrapping these for consistency, though this is optional as these system errors already contain sufficient context.♻️ Optional: Wrap system errors for consistency
func FindRegistry(m io.Medium) (string, error) { // Check current directory and parents dir, err := os.Getwd() if err != nil { - return "", err + return "", coreerr.E("repos.FindRegistry", "failed to get working directory", err) } ... // Check home directory common locations home, err := os.UserHomeDir() if err != nil { - return "", err + return "", coreerr.E("repos.FindRegistry", "failed to get home directory", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repos/registry.go` around lines 105 - 150, FindRegistry returns raw system errors from os.Getwd() and os.UserHomeDir() inconsistent with the rest of the file; wrap those errors with coreerr.E to provide structured context. Replace the direct returns of err after os.Getwd() and os.UserHomeDir() with coreerr.E calls (e.g. coreerr.E("repos.FindRegistry", "unable to get current working directory", err) and coreerr.E("repos.FindRegistry", "unable to get user home directory", err)) so error handling matches other paths and includes the original error as the cause.go.mod (1)
3-3: Go 1.26.0 is released and available. Go 1.26.1 (released 5 March 2026) is now the latest patch release; consider upgrading for bug fixes if deployment targets support it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, The go.mod currently pins the Go language version to "go 1.26.0"; update the module directive to the latest patch release by changing that value to "go 1.26.1" so the project uses the 1.26.1 patch level (edit the go directive string in go.mod from 1.26.0 to 1.26.1).cmd/scm/cmd_scm.go (1)
14-16: Inconsistent localisation setup compared tocmd/forge/cmd_forge.go.The forge command module passes
locales.FStocli.RegisterCommands, but this SCM module does not. If localisation is intended for SCM commands, consider passinglocales.FShere as well for consistency.♻️ Suggested change for consistency
import ( "forge.lthn.ai/core/cli/pkg/cli" + "forge.lthn.ai/core/go-scm/locales" ) func init() { - cli.RegisterCommands(AddScmCommands) + cli.RegisterCommands(AddScmCommands, locales.FS) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/scm/cmd_scm.go` around lines 14 - 16, Init in cmd_scm.go currently calls cli.RegisterCommands(AddScmCommands) without the localization FS; update the call to pass locales.FS like the forge module does so localization is enabled for SCM commands — locate the init function and change the call to cli.RegisterCommands to include the second argument locales.FS while keeping AddScmCommands as the first argument.marketplace/builder.go (1)
116-125: Consider accepting anio.Mediumparameter for testability.
WriteIndexandloadFromDirusecoreio.Localdirectly, unlike other code in this PR which injectsio.Mediumfor testability. This makes unit testing these functions harder without touching the real filesystem.♻️ Suggested approach
Add a
Mediumfield toBuilderand use it inloadFromDir:type Builder struct { + Medium io.Medium // Filesystem abstraction; defaults to coreio.Local BaseURL string Org string }For
WriteIndex, consider accepting a Medium parameter:-func WriteIndex(path string, idx *Index) error { +func WriteIndex(medium io.Medium, path string, idx *Index) error {Also applies to: 128-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@marketplace/builder.go` around lines 116 - 125, WriteIndex and loadFromDir directly use coreio.Local which hinders testing; add an io.Medium dependency to Builder (e.g., a Medium field) and update loadFromDir to use b.Medium instead of coreio.Local, and change WriteIndex to accept a Medium parameter (or make it a Builder method that uses the Builder.Medium) so tests can inject a fake Medium; update all call sites (including any tests) to pass/inject the Medium and keep existing error handling (refer to functions WriteIndex, loadFromDir and the Builder type and coreio.Local symbol to locate changes).marketplace/installer.go (1)
107-116: Consider logging or returning the deletion error.The
DeleteAllerror is silently ignored. If directory removal fails, the store entry is still deleted, which could leave orphaned files on disk. Consider logging the error or returning a partial-success error.♻️ Optional: Log the deletion error
dest := filepath.Join(i.modulesDir, code) - _ = i.medium.DeleteAll(dest) + if err := i.medium.DeleteAll(dest); err != nil { + // Log but continue — prefer removing store entry even if files linger. + // Alternatively: return coreerr.E("marketplace.Installer.Remove", "delete files", err) + } return i.store.Delete(storeGroup, code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@marketplace/installer.go` around lines 107 - 116, The call to i.medium.DeleteAll(dest) in Installer.Remove is ignoring its error which can leave orphaned files while still deleting the store record; update Installer.Remove to capture the error from i.medium.DeleteAll(dest), and either log it (via an existing logger on Installer) and return a composite/partial-success error or return the deletion error (or a wrapped error) so the caller knows removal from disk failed before calling i.store.Delete(storeGroup, code); ensure you reference and handle errors from i.medium.DeleteAll and decide whether to still call i.store.Delete only on successful deletion or to return a combined error if you proceed.cmd/scm/cmd_export.go (1)
47-48: Inconsistent error wrapping.The error from
manifest.Compileis returned directly, whereas other errors in this function are wrapped withcli.WrapVerb. Consider wrapping for consistency:cm, err = manifest.Compile(m, manifest.CompileOptions{ Commit: gitCommit(dir), Tag: gitTag(dir), BuiltBy: "core scm export", }) if err != nil { - return err + return cli.WrapVerb(err, "compile", "manifest") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/scm/cmd_export.go` around lines 47 - 48, Wrap the error returned from manifest.Compile with cli.WrapVerb for consistency with other errors in this function: locate the call to manifest.Compile (and its surrounding if err != nil { return err }) in cmd_export.go and replace the direct return with returning cli.WrapVerb(err, "compiling manifest") (or similar contextual message) so the error matches the wrapping used elsewhere in this function.jobrunner/handlers/dispatch.go (1)
110-116: Keep the early-successActionResultconsistent with the other success exits.This branch now records timing, but it still omits
RepoOwner,RepoName,EpicNumber, andChildNumberwhile the other success paths populate them. Filling the same fields here will keep result handling and metrics consistent.🔧 Possible fix
return &jobrunner.ActionResult{ - Action: "dispatch", - Success: true, - Timestamp: time.Now(), - Duration: time.Since(start), + Action: "dispatch", + RepoOwner: safeOwner, + RepoName: safeRepo, + EpicNumber: signal.EpicNumber, + ChildNumber: signal.ChildNumber, + Success: true, + Timestamp: time.Now(), + Duration: time.Since(start), }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobrunner/handlers/dispatch.go` around lines 110 - 116, The early-success return in dispatch (the ActionResult returned after detecting "issue already processed") is missing repo and issue identifiers; update that returned &jobrunner.ActionResult to include RepoOwner, RepoName, EpicNumber, and ChildNumber (use the existing signal.RepoOwner, signal.RepoName, signal.EpicNumber, signal.ChildNumber variables) alongside the existing Action, Success, Timestamp and Duration so it matches the other success exits and keeps metrics/result handling consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/scm/cmd_compile.go`:
- Around line 54-60: Validate the decoded signing key length before casting: in
the block handling signKeyHex (where keyBytes, err :=
hex.DecodeString(signKeyHex) and opts.SignKey = ed25519.PrivateKey(keyBytes) are
used), check that len(keyBytes) is either 32 or 64 and return a wrapped error
(using cli.WrapVerb) if not; if valid, proceed to set opts.SignKey; add "fmt" to
imports if you need to format the error message.
In `@cmd/scm/cmd_index.go`:
- Around line 50-53: The call to filepath.Abs(output) currently discards its
error which can hide path resolution failures; update the code that sets
absOutput to check the error returned by filepath.Abs(output) and handle it
(e.g., return the error) or explicitly fall back to using the original output
value before calling marketplace.WriteIndex(absOutput, idx); ensure you
reference the filepath.Abs call, the absOutput variable, and the subsequent
marketplace.WriteIndex(absOutput, idx) so the change is applied in the same
block.
In `@docs/plans/2026-03-15-manifest-core-json.md`:
- Around line 1-414: The plan is outdated: implementation uses Compile(m
*Manifest, opts CompileOptions) (*CompiledManifest, error) instead of the
planned Compile(medium io.Medium, root string, opts CompileOptions) error,
marketplace file is builder.go not indexer.go, errors use coreerr.E not
fmt.Errorf, and checklist boxes remain unchecked; update the document to reflect
current reality by changing the function signature examples to match Compile and
CompileOptions as implemented, replace references to marketplace/indexer.go with
marketplace/builder.go and BuildIndex with the actual builder API, note that
errors use coreerr.E (or update implementation if you prefer fmt.Errorf), and
mark completed checkboxes (or archive the plan) so future readers aren’t misled.
In `@jobrunner/handlers/dispatch.go`:
- Around line 90-96: SanitizePath currently mutates inputs like "foo/bar" into
"bar"; update the dispatch.Execute flow to reject repo identifiers that change
during sanitization: call agentci.SanitizePath for signal.RepoOwner and
signal.RepoName, compare the sanitized results (safeOwner/safeRepo) to the
originals (signal.RepoOwner/signal.RepoName) and return an error via
coreerr.E("dispatch.Execute", "invalid repo owner/name", ...) if they differ;
also ensure any error paths such as failDispatch consistently use the sanitized
safeOwner/safeRepo values (and not the raw signal fields) so actions
(dispatch/label/comment) cannot target a different repo.
- Around line 264-266: The code returns a detailed error from
cmd.CombinedOutput() that exposes agent.Host and raw SSH output; change it to
log the full diagnostics (host and output) to the local logger at debug/error
level, but return a redacted generic error from coreerr.E (e.g.
"dispatch.transfer: ssh transfer failed") so callers like failDispatch don't
receive internal hostnames or shell output. Locate the CombinedOutput() call and
the coreerr.E("dispatch.transfer", ...) return and replace the returned message
with a non-sensitive generic string while emitting the original fmt.Sprintf(...)
and output to your existing logger before returning.
In `@manifest/compile.go`:
- Around line 16-24: Remove the non-standard JSON inline tag from the embedded
Manifest field in CompiledManifest: update the struct declaration for
CompiledManifest (the Manifest embedded field) to drop `json:",inline"` while
keeping `yaml:",inline"` so YAML behavior remains unchanged; ensure the struct
still includes Commit, Tag, BuiltAt, and BuiltBy as before.
---
Nitpick comments:
In `@cmd/scm/cmd_export.go`:
- Around line 47-48: Wrap the error returned from manifest.Compile with
cli.WrapVerb for consistency with other errors in this function: locate the call
to manifest.Compile (and its surrounding if err != nil { return err }) in
cmd_export.go and replace the direct return with returning cli.WrapVerb(err,
"compiling manifest") (or similar contextual message) so the error matches the
wrapping used elsewhere in this function.
In `@cmd/scm/cmd_scm.go`:
- Around line 14-16: Init in cmd_scm.go currently calls
cli.RegisterCommands(AddScmCommands) without the localization FS; update the
call to pass locales.FS like the forge module does so localization is enabled
for SCM commands — locate the init function and change the call to
cli.RegisterCommands to include the second argument locales.FS while keeping
AddScmCommands as the first argument.
In `@forge/labels.go`:
- Around line 80-91: GetLabelByName currently returns any error and EnsureLabel
treats them as "not found"; change this so GetLabelByName (or the underlying
ListRepoLabels) returns a sentinel error (e.g., var ErrLabelNotFound =
errors.New("label not found")) or a typed error when the label truly does not
exist, then update EnsureLabel to only call CreateRepoLabel(owner, repo, ...)
when err == ErrLabelNotFound (or the error type matches) and to return the
original error for all other failures; update any callers/tests accordingly and
keep the unique symbols GetLabelByName, EnsureLabel, ListRepoLabels,
CreateRepoLabel in mind while making the change.
In `@go.mod`:
- Line 3: The go.mod currently pins the Go language version to "go 1.26.0";
update the module directive to the latest patch release by changing that value
to "go 1.26.1" so the project uses the 1.26.1 patch level (edit the go directive
string in go.mod from 1.26.0 to 1.26.1).
In `@jobrunner/handlers/dispatch.go`:
- Around line 110-116: The early-success return in dispatch (the ActionResult
returned after detecting "issue already processed") is missing repo and issue
identifiers; update that returned &jobrunner.ActionResult to include RepoOwner,
RepoName, EpicNumber, and ChildNumber (use the existing signal.RepoOwner,
signal.RepoName, signal.EpicNumber, signal.ChildNumber variables) alongside the
existing Action, Success, Timestamp and Duration so it matches the other success
exits and keeps metrics/result handling consistent.
In `@jobrunner/handlers/tick_parent.go`:
- Line 11: The import alias coreerr in tick_parent.go is misleading for the
package path forge.lthn.ai/core/go-log; verify the intended package and either
rename the alias to a clearer identifier (e.g., corelog or logpkg) and update
all usages of coreerr in this file (functions/methods that call coreerr.*), or
add a succinct inline comment next to the import explaining that this logging
package is used for error reporting; ensure the chosen alias matches other files
for consistency.
In `@marketplace/builder.go`:
- Around line 116-125: WriteIndex and loadFromDir directly use coreio.Local
which hinders testing; add an io.Medium dependency to Builder (e.g., a Medium
field) and update loadFromDir to use b.Medium instead of coreio.Local, and
change WriteIndex to accept a Medium parameter (or make it a Builder method that
uses the Builder.Medium) so tests can inject a fake Medium; update all call
sites (including any tests) to pass/inject the Medium and keep existing error
handling (refer to functions WriteIndex, loadFromDir and the Builder type and
coreio.Local symbol to locate changes).
In `@marketplace/installer.go`:
- Around line 107-116: The call to i.medium.DeleteAll(dest) in Installer.Remove
is ignoring its error which can leave orphaned files while still deleting the
store record; update Installer.Remove to capture the error from
i.medium.DeleteAll(dest), and either log it (via an existing logger on
Installer) and return a composite/partial-success error or return the deletion
error (or a wrapped error) so the caller knows removal from disk failed before
calling i.store.Delete(storeGroup, code); ensure you reference and handle errors
from i.medium.DeleteAll and decide whether to still call i.store.Delete only on
successful deletion or to return a combined error if you proceed.
In `@repos/registry.go`:
- Around line 105-150: FindRegistry returns raw system errors from os.Getwd()
and os.UserHomeDir() inconsistent with the rest of the file; wrap those errors
with coreerr.E to provide structured context. Replace the direct returns of err
after os.Getwd() and os.UserHomeDir() with coreerr.E calls (e.g.
coreerr.E("repos.FindRegistry", "unable to get current working directory", err)
and coreerr.E("repos.FindRegistry", "unable to get user home directory", err))
so error handling matches other paths and includes the original error as the
cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98715a8d-9902-4823-aaa1-d5d23d82d222
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
agentci/config.goagentci/security.gocmd/forge/cmd_forge.gocmd/forge/cmd_sync.gocmd/gitea/cmd_sync.gocmd/scm/cmd_compile.gocmd/scm/cmd_export.gocmd/scm/cmd_index.gocmd/scm/cmd_scm.godocs/architecture.mddocs/plans/2026-03-15-manifest-core-json.mdforge/labels.gogo.modjobrunner/handlers/completion.gojobrunner/handlers/dispatch.gojobrunner/handlers/resolve_threads.gojobrunner/handlers/tick_parent.gojobrunner/journal.golocales/embed.golocales/en.jsonmanifest/compile.gomanifest/compile_test.gomanifest/loader.gomanifest/manifest.gomanifest/sign.gomarketplace/builder.gomarketplace/builder_test.gomarketplace/discovery.gomarketplace/installer.gomarketplace/installer_test.gomarketplace/marketplace.goplugin/installer.gorepos/gitstate.gorepos/kbconfig.gorepos/registry.gorepos/workconfig.go
Comment on lines
+54
to
+60
| if signKeyHex != "" { | ||
| keyBytes, err := hex.DecodeString(signKeyHex) | ||
| if err != nil { | ||
| return cli.WrapVerb(err, "decode", "sign key") | ||
| } | ||
| opts.SignKey = ed25519.PrivateKey(keyBytes) | ||
| } |
There was a problem hiding this comment.
Missing key length validation before cast.
The hex-decoded bytes are cast directly to ed25519.PrivateKey without validating the length. An ed25519 private key should be 64 bytes (or 32 bytes for seed-only). Invalid lengths will cause cryptographic operations to fail or behave unexpectedly.
🛡️ Proposed fix to add validation
if signKeyHex != "" {
keyBytes, err := hex.DecodeString(signKeyHex)
if err != nil {
return cli.WrapVerb(err, "decode", "sign key")
}
+ if len(keyBytes) != ed25519.PrivateKeySize {
+ return cli.WrapVerb(fmt.Errorf("expected %d bytes, got %d", ed25519.PrivateKeySize, len(keyBytes)), "validate", "sign key")
+ }
opts.SignKey = ed25519.PrivateKey(keyBytes)
}Note: This requires adding "fmt" to the imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/scm/cmd_compile.go` around lines 54 - 60, Validate the decoded signing
key length before casting: in the block handling signKeyHex (where keyBytes, err
:= hex.DecodeString(signKeyHex) and opts.SignKey = ed25519.PrivateKey(keyBytes)
are used), check that len(keyBytes) is either 32 or 64 and return a wrapped
error (using cli.WrapVerb) if not; if valid, proceed to set opts.SignKey; add
"fmt" to imports if you need to format the error message.
Comment on lines
+50
to
+53
| absOutput, _ := filepath.Abs(output) | ||
| if err := marketplace.WriteIndex(absOutput, idx); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Discarded error from filepath.Abs could mask path resolution issues.
The error from filepath.Abs(output) is silently ignored. While Abs rarely fails, discarding the error means unexpected behaviour could go unnoticed. Consider either handling the error or falling back to the original output value explicitly.
🛡️ Suggested fix
- absOutput, _ := filepath.Abs(output)
- if err := marketplace.WriteIndex(absOutput, idx); err != nil {
+ absOutput, err := filepath.Abs(output)
+ if err != nil {
+ absOutput = output // fall back to relative path
+ }
+ if err := marketplace.WriteIndex(absOutput, idx); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| absOutput, _ := filepath.Abs(output) | |
| if err := marketplace.WriteIndex(absOutput, idx); err != nil { | |
| return err | |
| } | |
| absOutput, err := filepath.Abs(output) | |
| if err != nil { | |
| absOutput = output // fall back to relative path | |
| } | |
| if err := marketplace.WriteIndex(absOutput, idx); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/scm/cmd_index.go` around lines 50 - 53, The call to filepath.Abs(output)
currently discards its error which can hide path resolution failures; update the
code that sets absOutput to check the error returned by filepath.Abs(output) and
handle it (e.g., return the error) or explicitly fall back to using the original
output value before calling marketplace.WriteIndex(absOutput, idx); ensure you
reference the filepath.Abs call, the absOutput variable, and the subsequent
marketplace.WriteIndex(absOutput, idx) so the change is applied in the same
block.
Comment on lines
+1
to
+414
| # Manifest → core.json Pipeline Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Add a build step that compiles `.core/manifest.yaml` into a `core.json` distribution artifact at the repo root, and a catalogue generator that indexes `core.json` files across repos into a marketplace index. | ||
|
|
||
| **Architecture:** `manifest.Compile()` reads `.core/manifest.yaml`, injects version/commit metadata, and writes `core.json` at the distribution root. `marketplace.BuildIndex()` crawls a repos registry (or forge org), extracts manifests, and produces an `index.json` catalogue. Both use go-io Medium for filesystem abstraction. Tests use `io.NewMockMedium()`. | ||
|
|
||
| **Tech Stack:** Go, go-io Medium, go-scm manifest/marketplace packages, testify | ||
|
|
||
| --- | ||
|
|
||
| ## File Structure | ||
|
|
||
| | File | Action | Purpose | | ||
| |------|--------|---------| | ||
| | `manifest/compile.go` | Create | `Compile()` — manifest.yaml → core.json with build metadata | | ||
| | `manifest/compile_test.go` | Create | Tests for compilation, metadata injection, signing | | ||
| | `marketplace/indexer.go` | Create | `BuildIndex()` — crawl repos, extract manifests, build catalogue | | ||
| | `marketplace/indexer_test.go` | Create | Tests for indexing, dedup, category extraction | | ||
|
|
||
| --- | ||
|
|
||
| ## Task 1: Manifest Compilation (manifest.yaml → core.json) | ||
|
|
||
| **Files:** | ||
| - Create: `manifest/compile.go` | ||
| - Create: `manifest/compile_test.go` | ||
|
|
||
| The `Compile` function reads `.core/manifest.yaml`, injects build metadata (version, commit, build time), and writes `core.json` at the target root. The output is JSON (not YAML) so consumers don't need a YAML parser. | ||
|
|
||
| - [ ] **Step 1: Write the failing test for Compile** | ||
|
|
||
| ```go | ||
| // compile_test.go | ||
| package manifest | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| io "forge.lthn.ai/core/go-io" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestCompile_Good(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| // Write a manifest.yaml | ||
| manifest := `code: core/api | ||
| name: Core API | ||
| version: 0.1.0 | ||
| namespace: api | ||
| binary: ./bin/core-api | ||
| licence: EUPL-1.2 | ||
| ` | ||
| medium.WriteString("/project/.core/manifest.yaml", manifest) | ||
|
|
||
| // Compile with build metadata | ||
| err := Compile(medium, "/project", CompileOptions{ | ||
| Version: "1.2.3", | ||
| Commit: "abc1234", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Read core.json | ||
| data, err := medium.Read("/project/core.json") | ||
| require.NoError(t, err) | ||
|
|
||
| var result CompiledManifest | ||
| require.NoError(t, json.Unmarshal([]byte(data), &result)) | ||
|
|
||
| assert.Equal(t, "core/api", result.Code) | ||
| assert.Equal(t, "Core API", result.Name) | ||
| assert.Equal(t, "1.2.3", result.Version) | ||
| assert.Equal(t, "abc1234", result.Commit) | ||
| assert.Equal(t, "api", result.Namespace) | ||
| assert.NotEmpty(t, result.BuiltAt) | ||
| } | ||
|
|
||
| func TestCompile_Good_PreservesSign(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| manifest := `code: core/api | ||
| name: Core API | ||
| version: 0.1.0 | ||
| sign: "dGVzdHNpZw==" | ||
| ` | ||
| medium.WriteString("/project/.core/manifest.yaml", manifest) | ||
|
|
||
| err := Compile(medium, "/project", CompileOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| data, err := medium.Read("/project/core.json") | ||
| require.NoError(t, err) | ||
|
|
||
| var result CompiledManifest | ||
| require.NoError(t, json.Unmarshal([]byte(data), &result)) | ||
|
|
||
| assert.Equal(t, "dGVzdHNpZw==", result.Sign) | ||
| } | ||
|
|
||
| func TestCompile_Bad_NoManifest(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| err := Compile(medium, "/project", CompileOptions{}) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "manifest.Compile") | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Run test to verify it fails** | ||
|
|
||
| Run: `go test -v -run TestCompile ./manifest/` | ||
| Expected: FAIL — `Compile` undefined | ||
|
|
||
| - [ ] **Step 3: Write minimal implementation** | ||
|
|
||
| ```go | ||
| // compile.go | ||
| package manifest | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| io "forge.lthn.ai/core/go-io" | ||
| ) | ||
|
|
||
| // CompiledManifest is the core.json distribution format. | ||
| // Embeds the full Manifest plus build metadata. | ||
| type CompiledManifest struct { | ||
| Manifest | ||
|
|
||
| // Build metadata — injected at compile time, not in source manifest. | ||
| Commit string `json:"commit,omitempty"` | ||
| BuiltAt string `json:"built_at,omitempty"` | ||
| } | ||
|
|
||
| // CompileOptions controls what metadata is injected during compilation. | ||
| type CompileOptions struct { | ||
| Version string // Override version (e.g. from git tag) | ||
| Commit string // Git commit hash | ||
| Output string // Output path (default: "core.json" at root) | ||
| } | ||
|
|
||
| // Compile reads .core/manifest.yaml, injects build metadata, and writes | ||
| // core.json at the distribution root. | ||
| func Compile(medium io.Medium, root string, opts CompileOptions) error { | ||
| m, err := Load(medium, root) | ||
| if err != nil { | ||
| return fmt.Errorf("manifest.Compile: %w", err) | ||
| } | ||
|
|
||
| compiled := CompiledManifest{ | ||
| Manifest: *m, | ||
| Commit: opts.Commit, | ||
| BuiltAt: time.Now().UTC().Format(time.RFC3339), | ||
| } | ||
|
|
||
| // Override version if provided (e.g. from git tag) | ||
| if opts.Version != "" { | ||
| compiled.Version = opts.Version | ||
| } | ||
|
|
||
| data, err := json.MarshalIndent(compiled, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("manifest.Compile: marshal: %w", err) | ||
| } | ||
|
|
||
| outPath := opts.Output | ||
| if outPath == "" { | ||
| outPath = filepath.Join(root, "core.json") | ||
| } | ||
|
|
||
| if err := medium.Write(outPath, string(data)); err != nil { | ||
| return fmt.Errorf("manifest.Compile: write: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 4: Run test to verify it passes** | ||
|
|
||
| Run: `go test -v -run TestCompile ./manifest/` | ||
| Expected: PASS | ||
|
|
||
| - [ ] **Step 5: Commit** | ||
|
|
||
| ```bash | ||
| git add manifest/compile.go manifest/compile_test.go | ||
| git commit -m "feat(manifest): compile .core/manifest.yaml to core.json | ||
|
|
||
| Co-Authored-By: Virgil <virgil@lethean.io>" | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Task 2: Marketplace Index Builder | ||
|
|
||
| **Files:** | ||
| - Create: `marketplace/indexer.go` | ||
| - Create: `marketplace/indexer_test.go` | ||
|
|
||
| The `BuildIndex` function takes a list of directory paths (repos), loads each `.core/manifest.yaml`, extracts Module entries, deduplicates categories, and produces an `Index`. | ||
|
|
||
| - [ ] **Step 1: Write the failing test for BuildIndex** | ||
|
|
||
| ```go | ||
| // indexer_test.go | ||
| package marketplace | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| io "forge.lthn.ai/core/go-io" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestBuildIndex_Good(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| // Two repos with manifests | ||
| medium.WriteString("/repos/core-api/.core/manifest.yaml", ` | ||
| code: core/api | ||
| name: Core API | ||
| version: 0.1.0 | ||
| namespace: api | ||
| binary: ./bin/api | ||
| `) | ||
| medium.WriteString("/repos/core-bio/.core/manifest.yaml", ` | ||
| code: core/bio | ||
| name: Bio | ||
| version: 0.2.0 | ||
| namespace: bio | ||
| binary: ./bin/bio | ||
| `) | ||
|
|
||
| idx, err := BuildIndex(medium, []string{"/repos/core-api", "/repos/core-bio"}, IndexOptions{ | ||
| Org: "core", | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, 1, idx.Version) | ||
| assert.Len(t, idx.Modules, 2) | ||
| assert.Equal(t, "core/api", idx.Modules[0].Code) | ||
| assert.Equal(t, "core/bio", idx.Modules[1].Code) | ||
| } | ||
|
|
||
| func TestBuildIndex_Good_SkipsMissingManifest(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| // Only one repo has a manifest | ||
| medium.WriteString("/repos/core-api/.core/manifest.yaml", ` | ||
| code: core/api | ||
| name: Core API | ||
| version: 0.1.0 | ||
| `) | ||
|
|
||
| idx, err := BuildIndex(medium, []string{"/repos/core-api", "/repos/no-manifest"}, IndexOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Len(t, idx.Modules, 1) | ||
| } | ||
|
|
||
| func TestBuildIndex_Good_ExtractsCategories(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| medium.WriteString("/repos/a/.core/manifest.yaml", ` | ||
| code: a | ||
| name: A | ||
| `) | ||
| medium.WriteString("/repos/b/.core/manifest.yaml", ` | ||
| code: b | ||
| name: B | ||
| `) | ||
|
|
||
| idx, err := BuildIndex(medium, []string{"/repos/a", "/repos/b"}, IndexOptions{ | ||
| CategoryFn: func(code string) string { | ||
| if code == "a" { | ||
| return "tools" | ||
| } | ||
| return "products" | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Contains(t, idx.Categories, "tools") | ||
| assert.Contains(t, idx.Categories, "products") | ||
| } | ||
|
|
||
| func TestBuildIndex_Bad_EmptyList(t *testing.T) { | ||
| medium := io.NewMockMedium() | ||
|
|
||
| idx, err := BuildIndex(medium, []string{}, IndexOptions{}) | ||
| require.NoError(t, err) | ||
| assert.Len(t, idx.Modules, 0) | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Run test to verify it fails** | ||
|
|
||
| Run: `go test -v -run TestBuildIndex ./marketplace/` | ||
| Expected: FAIL — `BuildIndex` undefined | ||
|
|
||
| - [ ] **Step 3: Write minimal implementation** | ||
|
|
||
| ```go | ||
| // indexer.go | ||
| package marketplace | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "sort" | ||
|
|
||
| io "forge.lthn.ai/core/go-io" | ||
| "forge.lthn.ai/core/go-scm/manifest" | ||
| ) | ||
|
|
||
| // IndexOptions controls how the index is built. | ||
| type IndexOptions struct { | ||
| Org string // Git org for repo URLs (e.g. "core") | ||
| ForgeURL string // Forge base URL (default: "https://forge.lthn.ai") | ||
| CategoryFn func(code string) string // Optional function to assign category by code | ||
| } | ||
|
|
||
| // BuildIndex reads .core/manifest.yaml from each repo path and produces | ||
| // a marketplace Index. Repos without a manifest are silently skipped. | ||
| func BuildIndex(medium io.Medium, repoPaths []string, opts IndexOptions) (*Index, error) { | ||
| if opts.ForgeURL == "" { | ||
| opts.ForgeURL = "https://forge.lthn.ai" | ||
| } | ||
|
|
||
| idx := &Index{Version: 1} | ||
| seen := make(map[string]bool) | ||
| catSet := make(map[string]bool) | ||
|
|
||
| for _, repoPath := range repoPaths { | ||
| m, err := manifest.Load(medium, repoPath) | ||
| if err != nil { | ||
| continue // Skip repos without manifest | ||
| } | ||
|
|
||
| if m.Code == "" { | ||
| continue | ||
| } | ||
|
|
||
| if seen[m.Code] { | ||
| continue // Deduplicate | ||
| } | ||
| seen[m.Code] = true | ||
|
|
||
| module := Module{ | ||
| Code: m.Code, | ||
| Name: m.Name, | ||
| SignKey: m.Sign, | ||
| } | ||
|
|
||
| // Build repo URL | ||
| if opts.Org != "" { | ||
| module.Repo = fmt.Sprintf("%s/%s/%s.git", opts.ForgeURL, opts.Org, m.Code) | ||
| } | ||
|
|
||
| // Assign category | ||
| if opts.CategoryFn != nil { | ||
| module.Category = opts.CategoryFn(m.Code) | ||
| } | ||
| if module.Category != "" { | ||
| catSet[module.Category] = true | ||
| } | ||
|
|
||
| idx.Modules = append(idx.Modules, module) | ||
| } | ||
|
|
||
| // Sort categories | ||
| for cat := range catSet { | ||
| idx.Categories = append(idx.Categories, cat) | ||
| } | ||
| sort.Strings(idx.Categories) | ||
|
|
||
| return idx, nil | ||
| } | ||
| ``` | ||
|
|
||
| - [ ] **Step 4: Run test to verify it passes** | ||
|
|
||
| Run: `go test -v -run TestBuildIndex ./marketplace/` | ||
| Expected: PASS | ||
|
|
||
| - [ ] **Step 5: Commit** | ||
|
|
||
| ```bash | ||
| git add marketplace/indexer.go marketplace/indexer_test.go | ||
| git commit -m "feat(marketplace): index builder — crawl repos, build catalogue | ||
|
|
||
| Co-Authored-By: Virgil <virgil@lethean.io>" | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| **Total: 2 tasks, ~10 steps** | ||
|
|
||
| After completion: | ||
| - `manifest.Compile()` produces `core.json` at distribution root | ||
| - `marketplace.BuildIndex()` crawls repo paths and produces `index.json` | ||
| - Both are testable via mock Medium (no filesystem) | ||
| - Ready for integration into `core build` and `core scm index` CLI commands |
There was a problem hiding this comment.
Plan document is now outdated.
The implementation diverges from this plan in several ways:
-
Function signatures differ: The plan shows
Compile(medium io.Medium, root string, opts CompileOptions) error, but the actual implementation isCompile(m *Manifest, opts CompileOptions) (*CompiledManifest, error). -
File naming differs: The plan references
marketplace/indexer.go, but the implementation usesmarketplace/builder.go. -
Error handling differs: The plan uses
fmt.Errorf, but the implementation usescoreerr.E. -
Checkboxes are unchecked: All task checkboxes remain as
- [ ]despite the implementation being complete.
Consider updating or archiving this plan to avoid confusion for future developers or agentic workers who may attempt to follow it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-15-manifest-core-json.md` around lines 1 - 414, The plan
is outdated: implementation uses Compile(m *Manifest, opts CompileOptions)
(*CompiledManifest, error) instead of the planned Compile(medium io.Medium, root
string, opts CompileOptions) error, marketplace file is builder.go not
indexer.go, errors use coreerr.E not fmt.Errorf, and checklist boxes remain
unchecked; update the document to reflect current reality by changing the
function signature examples to match Compile and CompileOptions as implemented,
replace references to marketplace/indexer.go with marketplace/builder.go and
BuildIndex with the actual builder API, note that errors use coreerr.E (or
update implementation if you prefer fmt.Errorf), and mark completed checkboxes
(or archive the plan) so future readers aren’t misled.
Comment on lines
90
to
+96
| safeOwner, err := agentci.SanitizePath(signal.RepoOwner) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid repo owner: %w", err) | ||
| return nil, coreerr.E("dispatch.Execute", "invalid repo owner", err) | ||
| } | ||
| safeRepo, err := agentci.SanitizePath(signal.RepoName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid repo name: %w", err) | ||
| return nil, coreerr.E("dispatch.Execute", "invalid repo name", err) |
There was a problem hiding this comment.
Reject repo identifiers that change during sanitisation.
agentci.SanitizePath in agentci/security.go runs filepath.Base before validation, so foo/bar becomes bar instead of failing. In this handler that can dispatch, label, and comment against the wrong repo, and the later failDispatch path still uses the unsanitised owner/name.
🔧 Possible fix
safeOwner, err := agentci.SanitizePath(signal.RepoOwner)
- if err != nil {
+ if err != nil || safeOwner != signal.RepoOwner {
return nil, coreerr.E("dispatch.Execute", "invalid repo owner", err)
}
safeRepo, err := agentci.SanitizePath(signal.RepoName)
- if err != nil {
+ if err != nil || safeRepo != signal.RepoName {
return nil, coreerr.E("dispatch.Execute", "invalid repo name", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jobrunner/handlers/dispatch.go` around lines 90 - 96, SanitizePath currently
mutates inputs like "foo/bar" into "bar"; update the dispatch.Execute flow to
reject repo identifiers that change during sanitization: call
agentci.SanitizePath for signal.RepoOwner and signal.RepoName, compare the
sanitized results (safeOwner/safeRepo) to the originals
(signal.RepoOwner/signal.RepoName) and return an error via
coreerr.E("dispatch.Execute", "invalid repo owner/name", ...) if they differ;
also ensure any error paths such as failDispatch consistently use the sanitized
safeOwner/safeRepo values (and not the raw signal fields) so actions
(dispatch/label/comment) cannot target a different repo.
Comment on lines
264
to
+266
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return log.E("dispatch.transfer", fmt.Sprintf("ssh to %s failed: %s", agent.Host, string(output)), err) | ||
| return coreerr.E("dispatch.transfer", fmt.Sprintf("ssh to %s failed: %s", agent.Host, string(output)), err) |
There was a problem hiding this comment.
Redact SSH failure details before returning them.
This error text includes agent.Host and raw SSH output. Both transfer failure paths feed %v from this error into failDispatch, so internal hostnames and shell diagnostics can leak into the issue comment. Keep the verbose diagnostics in logs, but return a generic transfer error here.
🔧 Possible fix
output, err := cmd.CombinedOutput()
if err != nil {
- return coreerr.E("dispatch.transfer", fmt.Sprintf("ssh to %s failed: %s", agent.Host, string(output)), err)
+ coreerr.Warn("dispatch.transfer ssh failed", "agent", agent.Host, "output", string(bytes.TrimSpace(output)), "err", err)
+ return coreerr.E("dispatch.transfer", "ssh transfer failed", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return log.E("dispatch.transfer", fmt.Sprintf("ssh to %s failed: %s", agent.Host, string(output)), err) | |
| return coreerr.E("dispatch.transfer", fmt.Sprintf("ssh to %s failed: %s", agent.Host, string(output)), err) | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| coreerr.Warn("dispatch.transfer ssh failed", "agent", agent.Host, "output", string(bytes.TrimSpace(output)), "err", err) | |
| return coreerr.E("dispatch.transfer", "ssh transfer failed", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jobrunner/handlers/dispatch.go` around lines 264 - 266, The code returns a
detailed error from cmd.CombinedOutput() that exposes agent.Host and raw SSH
output; change it to log the full diagnostics (host and output) to the local
logger at debug/error level, but return a redacted generic error from coreerr.E
(e.g. "dispatch.transfer: ssh transfer failed") so callers like failDispatch
don't receive internal hostnames or shell output. Locate the CombinedOutput()
call and the coreerr.E("dispatch.transfer", ...) return and replace the returned
message with a non-sensitive generic string while emitting the original
fmt.Sprintf(...) and output to your existing logger before returning.
Comment on lines
+16
to
+24
| type CompiledManifest struct { | ||
| Manifest `json:",inline" yaml:",inline"` | ||
|
|
||
| // Build metadata — populated by Compile. | ||
| Commit string `json:"commit,omitempty" yaml:"commit,omitempty"` | ||
| Tag string `json:"tag,omitempty" yaml:"tag,omitempty"` | ||
| BuiltAt string `json:"built_at,omitempty" yaml:"built_at,omitempty"` | ||
| BuiltBy string `json:"built_by,omitempty" yaml:"built_by,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Go encoding/json support inline tag for embedded structs?
💡 Result:
No—encoding/json does not support a ,inline struct-tag option.
What it does support is field inlining via anonymous (embedded) structs: exported fields of an embedded struct are marshaled/unmarshaled as if they were fields of the outer struct (subject to the usual field-name conflict rules). (pkg.go.dev)
So:
type Inner struct { A int `json:"a"` }
type Outer struct {
Inner // embedded => inlined (no tag needed / no ",inline" exists)
B int `json:"b"`
}If you need to inline fields from a non-embedded field or want an explicit ,inline tag behavior, you’ll need custom MarshalJSON/UnmarshalJSON or a third-party helper (e.g., a library that implements ,inline on top of encoding/json). (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/encoding/json%40go1.26.1?utm_source=openai
- 2: https://pkg.go.dev/github.com/hydrz/jsoninline?utm_source=openai
🏁 Script executed:
# Check the full structure of CompiledManifest and surrounding code
cat -n manifest/compile.go | head -60
# Look for any custom MarshalJSON implementations
rg "MarshalJSON|UnmarshalJSON" manifest/Repository: dAppCore/go-scm
Length of output: 2733
🏁 Script executed:
# Get the MarshalJSON implementation to understand custom marshaling
cat -n manifest/compile.go | sed -n '62,100p'
# Check if there are more lines after 60
wc -l manifest/compile.go
# Get the full MarshalJSON function
rg -A 20 "func MarshalJSON" manifest/compile.goRepository: dAppCore/go-scm
Length of output: 2397
Remove the json:",inline" tag from the Manifest field.
The json:",inline" tag is not a standard encoding/json feature—it will be silently ignored. Go automatically inlines embedded struct fields in JSON marshalling regardless of tags, so this tag is redundant. The code works correctly as-is, but removing it improves clarity:
Suggested change
type CompiledManifest struct {
- Manifest `json:",inline" yaml:",inline"`
+ Manifest `yaml:",inline"`The yaml:",inline" tag should be kept, as it is required for proper YAML marshalling with gopkg.in/yaml.v3.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type CompiledManifest struct { | |
| Manifest `json:",inline" yaml:",inline"` | |
| // Build metadata — populated by Compile. | |
| Commit string `json:"commit,omitempty" yaml:"commit,omitempty"` | |
| Tag string `json:"tag,omitempty" yaml:"tag,omitempty"` | |
| BuiltAt string `json:"built_at,omitempty" yaml:"built_at,omitempty"` | |
| BuiltBy string `json:"built_by,omitempty" yaml:"built_by,omitempty"` | |
| } | |
| type CompiledManifest struct { | |
| Manifest `yaml:",inline"` | |
| // Build metadata — populated by Compile. | |
| Commit string `json:"commit,omitempty" yaml:"commit,omitempty"` | |
| Tag string `json:"tag,omitempty" yaml:"tag,omitempty"` | |
| BuiltAt string `json:"built_at,omitempty" yaml:"built_at,omitempty"` | |
| BuiltBy string `json:"built_by,omitempty" yaml:"built_by,omitempty"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@manifest/compile.go` around lines 16 - 24, Remove the non-standard JSON
inline tag from the embedded Manifest field in CompiledManifest: update the
struct declaration for CompiledManifest (the Manifest embedded field) to drop
`json:",inline"` while keeping `yaml:",inline"` so YAML behavior remains
unchanged; ensure the struct still includes Commit, Tag, BuiltAt, and BuiltBy as
before.
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.
Forge → GitHub Sync
Commits: 14
Files changed: 37
Automated sync from Forge (forge.lthn.ai) to GitHub mirror.
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
Tests