Skip to content

feat: store source_url on harness-config/template import + reimport CLI & UI#263

Open
ptone wants to merge 20 commits into
mainfrom
scion/harness-journey-p1
Open

feat: store source_url on harness-config/template import + reimport CLI & UI#263
ptone wants to merge 20 commits into
mainfrom
scion/harness-journey-p1

Conversation

@ptone

@ptone ptone commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add source_url field to HarnessConfig and Template Ent schemas, persisting the import origin URL through the entire import pipeline (ResourceRecord → Bootstrap → persistence → API responses)
  • New Hub endpoint POST /api/v1/harness-configs/{id}/reimport to re-fetch from stored source URL (with optional override)
  • New CLI command scion harness-config update <name> [--url <override>] [--all] for reimporting from stored source
  • "Refresh from Source" button on harness-config detail page, visible when source_url is set
  • Displays source_url metadata on the detail page

This is Phase 1 of the harness-journey design (.design/harness-journey.md), unblocking the update/reimport flow that was previously impossible because the import source URL was discarded after initial fetch.

Test plan

  • Import a harness config from URL → verify source_url is persisted (check via scion harness-config show <name>)
  • Import from workspace path → verify source_url is empty
  • Re-import via CLI: scion harness-config update <name> → verify content refreshed from stored URL
  • Re-import with override: scion harness-config update <name> --url <new-url> → verify new URL stored
  • --all flag: updates all configs with stored source URLs
  • Error cases: no source_url + no --url → clear error; --all + --url → error
  • UI: detail page shows source_url and "Refresh from Source" button
  • UI: button hidden when source_url is empty
  • Regression: normal import flow still works unchanged

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the ability to re-import and update harness configurations from their remote source URLs. It adds a new harness-config update CLI command, backend API endpoints, database schema updates for tracking source_url on templates and harness configurations, and a frontend 'Refresh from Source' button. The review feedback highlights three key areas for improvement: addressing a 'fail-open' authorization check in the re-import handler to ensure secure-by-default behavior, explicitly rejecting the combination of a specific configuration name and the --all flag in the CLI, and properly returning an error exit code when bulk updates encounter failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +739 to +754
if hc.Scope == store.HarnessConfigScopeGlobal {
userIdent := GetUserIdentityFromContext(ctx)
if userIdent == nil {
writeError(w, http.StatusUnauthorized, "unauthorized", "Authentication required", nil)
return
}
decision := s.authzService.CheckAccess(ctx, userIdent, Resource{Type: "harness_config"}, ActionCreate)
if !decision.Allowed {
writeError(w, http.StatusForbidden, ErrCodeForbidden, "You do not have permission to reimport global resources", nil)
return
}
} else if hc.Scope == store.HarnessConfigScopeProject {
if !s.authorizeProjectImport(ctx, w, hc.ScopeID, "harness-configs") {
return
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The authorization check is currently "fail-open". If hc.Scope is neither HarnessConfigScopeGlobal nor HarnessConfigScopeProject (for example, if it is empty or an unexpected value), the code will bypass all authorization checks and proceed with the reimport.

To adhere to secure coding practices, the authorization logic should be "fail-closed" by adding a default else block that rejects the request.

	if hc.Scope == store.HarnessConfigScopeGlobal {
		userIdent := GetUserIdentityFromContext(ctx)
		if userIdent == nil {
			writeError(w, http.StatusUnauthorized, "unauthorized", "Authentication required", nil)
			return
		}
		decision := s.authzService.CheckAccess(ctx, userIdent, Resource{Type: "harness_config"}, ActionCreate)
		if !decision.Allowed {
			writeError(w, http.StatusForbidden, ErrCodeForbidden, "You do not have permission to reimport global resources", nil)
			return
		}
	} else if hc.Scope == store.HarnessConfigScopeProject {
		if !s.authorizeProjectImport(ctx, w, hc.ScopeID, "harness-configs") {
			return
		}
	} else {
		writeError(w, http.StatusForbidden, "forbidden", "Unsupported or invalid harness-config scope", nil)
		return
	}

Comment on lines +48 to +53
if len(args) == 0 && !all {
return fmt.Errorf("specify a harness-config name or use --all")
}
if all && urlOverride != "" {
return fmt.Errorf("--all and --url cannot be used together")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If both a harness-config name (via args) and the --all flag are provided, the command will silently ignore the name and update all configs. This can lead to accidental bulk updates when the user only intended to update a single config.

We should explicitly reject the command if both a name and --all are specified.

	if len(args) == 0 && !all {
		return fmt.Errorf("specify a harness-config name or use --all")
	}
	if len(args) > 0 && all {
		return fmt.Errorf("cannot specify a harness-config name when using --all")
	}
	if all && urlOverride != "" {
		return fmt.Errorf("--all and --url cannot be used together")
	}

Comment thread cmd/harness_config_update.go Outdated
Comment on lines +163 to +172
if isJSONOutput() {
return outputJSON(ActionResult{
Status: "success",
Command: "harness-config update --all",
Message: fmt.Sprintf("Updated %d, skipped %d (no source URL), failed %d", updated, skipped, failed),
})
}

fmt.Printf("\nUpdated %d, skipped %d (no source URL), failed %d\n", updated, skipped, failed)
return nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If any harness-config updates fail during a bulk update (--all), the command still returns nil (exit code 0) and the JSON output reports Status: "success". This makes it difficult for scripts or CI/CD pipelines to detect failures.

We should return an error if failed > 0, and set the JSON status to "error" to properly reflect the partial or total failure.

	if isJSONOutput() {
		status := "success"
		if failed > 0 {
			status = "error"
		}
		if err := outputJSON(ActionResult{
			Status:  status,
			Command: "harness-config update --all",
			Message: fmt.Sprintf("Updated %d, skipped %d (no source URL), failed %d", updated, skipped, failed),
		}); err != nil {
			return err
		}
		if failed > 0 {
			return fmt.Errorf("failed to update %d harness-config(s)", failed)
		}
		return nil
	}

	fmt.Printf("\nUpdated %d, skipped %d (no source URL), failed %d\n", updated, skipped, failed)
	if failed > 0 {
		return fmt.Errorf("failed to update %d harness-config(s)", failed)
	}
	return nil

@ptone ptone force-pushed the scion/harness-journey-p1 branch 3 times, most recently from 2b216cf to 3cdd121 Compare June 18, 2026 15:30
ptone and others added 2 commits June 18, 2026 23:17
GoogleCloudPlatform#443)

Per-project import handlers now support NDJSON streaming when the client
sends Accept: application/x-ndjson, matching the unified endpoint. This
fixes the progress display (per-resource events) and the post-import
summary (the NDJSON done event uses the correct "imported" field name).

Also passes discovered names to executeImport even for the count===1
case, ensuring the import is scoped to the exact resource that was
discovered.

Co-authored-by: Scion Agent (template-import-selector-lead) <agent@scion.dev>
…orm#445)

* Warn when server binary is built without web assets

Add a prominent startup warning when web assets are not embedded and
no --web-assets-dir is provided. Serve the self-contained noAssetsPage
HTML from the static asset handler instead of a plain text 404, so
direct requests to /assets/* also show a helpful message.

* Use slog.Warn for missing web assets startup warning

Switch from log.Printf to slog.Warn so the warning formats correctly
in structured logging environments.

---------

Co-authored-by: Scion Agent (web-assets-warning-lead) <agent@scion.dev>
@ptone ptone force-pushed the scion/harness-journey-p1 branch from 0414107 to 69f09cd Compare June 18, 2026 22:32
ptone and others added 7 commits June 18, 2026 23:35
…Platform#444)

* Sync built image reference back to Hub after scion build

After building a harness config image, the new image tag was written to the
on-disk config.yaml but never synced to Hub. At agent start, Hub hydrated
from its stale stored copy (still pointing at the old upstream image), so
the locally-built image was never used.

Two fixes:

1. cmd/build.go: After updating on-disk config.yaml, auto-sync to Hub via
   syncHarnessConfigToHub(). If Hub is unavailable, print a warning
   suggesting the user run 'scion harness-config push'.

2. pkg/hub/maintenance_executors.go: After a successful build in
   BuildHarnessConfigImageExecutor, update the HarnessConfig record's
   Config.Image field in the DB and re-upload the modified config.yaml
   to Hub storage.

* Fix syncBuiltImage to update file manifest and content hash

After uploading the updated config.yaml to storage, recalculate the
hc.Files manifest entry (size + SHA-256 hash) and the overall
hc.ContentHash before writing the DB record. Without this, the Runtime
Broker's cache-invalidation check sees a stale ContentHash and never
pulls the updated config, and subsequent downloads fail manifest
validation against stale size/hash values.

Also validate that config.yaml's root node is a YAML mapping before
processing, returning an error instead of silently skipping malformed
files.

---------

Co-authored-by: Scion Agent (image-build-fix) <agent@scion.dev>
…atform#448)

Only set channel="web" when the authenticated user's ClientType is
"web", not for CLI or API callers hitting the same endpoint. Also
update stale doc-comment on the stream handler.

Co-authored-by: Scion Agent (msg-cleanups-lead) <agent@scion.dev>
…CloudPlatform#442)

* fix(image-build): use default builder for local docker builds

Using a custom docker-container builder (scion-builder) prevents BuildKit from resolving intermediate images built in previous steps when PUSH is false, because the container driver doesn't share the host's daemon image cache. Switching to the default builder for local builds resolves this.

* fix(image-build): use BUILDX_BUILDER env var to avoid mutating global config

Address PR GoogleCloudPlatform#442 reviewer feedback:
- Use the BUILDX_BUILDER environment variable instead of 'docker buildx use' to prevent mutating the user's global Docker CLI configuration.
- For push builds, export BUILDX_BUILDER="${BUILDX_INSTANCE}" and remove the --use flag from 'docker buildx create'.

---------

Co-authored-by: Preston Holmes <ptone@google.com>
…dPlatform#438)

Bumps [astro](https://github.com/withastro/astro/tree/HEAD/packages/astro) from 6.3.2 to 6.4.8.
- [Release notes](https://github.com/withastro/astro/releases)
- [Changelog](https://github.com/withastro/astro/blob/astro@6.4.8/packages/astro/CHANGELOG.md)
- [Commits](https://github.com/withastro/astro/commits/astro@6.4.8/packages/astro)

---
updated-dependencies:
- dependency-name: astro
  dependency-version: 6.4.8
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…Platform#439)

Bumps [dompurify](https://github.com/cure53/DOMPurify) from 3.4.9 to 3.4.11.
- [Release notes](https://github.com/cure53/DOMPurify/releases)
- [Commits](cure53/DOMPurify@3.4.9...3.4.11)

---
updated-dependencies:
- dependency-name: dompurify
  dependency-version: 3.4.11
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…oudPlatform#440)

Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 4.1.1 to 4.2.0.
- [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md)
- [Commits](https://github.com/nodeca/js-yaml/commits)

---
updated-dependencies:
- dependency-name: js-yaml
  dependency-version: 4.2.0
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…gleCloudPlatform#441)

Bumps [github.com/rclone/rclone](https://github.com/rclone/rclone) from 1.73.5 to 1.74.3.
- [Release notes](https://github.com/rclone/rclone/releases)
- [Changelog](https://github.com/rclone/rclone/blob/master/RELEASE.md)
- [Commits](rclone/rclone@v1.73.5...v1.74.3)

---
updated-dependencies:
- dependency-name: github.com/rclone/rclone
  dependency-version: 1.74.3
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ptone ptone force-pushed the scion/harness-journey-p1 branch from 91c9d15 to 73daa89 Compare June 18, 2026 22:49
Scion Agent (harness-journey-p1-dev) added 11 commits June 18, 2026 22:49
Add optional source_url field to track the import origin URL for
harness-configs and templates, enabling reimport/update flows.
… pipeline

Add SourceURL field to store.HarnessConfig, store.Template,
ResourceRecord, and the ent adapter CRUD operations. Update
Bootstrap() to accept sourceURL parameter and persist it on
create/update. Thread sourceURL from resourceDir through the
import worker loop.
- POST /api/v1/harness-configs/{id}/reimport endpoint that re-imports
  from stored source_url or an override URL
- CLI: scion harness-config update <name> [--url] [--all]
- Hub client: Reimport() method on HarnessConfigService
- Show command now displays source URL when present
- Display source URL as clickable link in metadata row
- Show Refresh from Source button when sourceUrl is present
- Button calls POST /api/v1/harness-configs/{id}/reimport
- Reload config details after successful reimport
The mock was missing the new Reimport method added to the
HarnessConfigService interface, causing CI lint failures.
Remove WithDropColumn and WithDropIndex from Ent auto-migration.
These flags cause SQLite to silently drop data during table recreation
when schema changes occur between versions, which was the root cause
of URL-imported harness configs disappearing after server restarts.
The importFromRemote signature gained a nameFilter []string parameter
upstream. Pass nil for the reimport endpoint since it reimports all
resources from the source URL.
Without a no_auth section, selecting "no authentication" for the
antigravity harness causes the container to start with the full
agy-wrapper.sh command (dbus + gnome-keyring + AGY) instead of
dropping to a shell. This fails because AGY has no credentials.

Add no_auth behavior matching claude/gemini/codex/opencode harnesses.
C-1: Add user-scope authorization check in reimport handler. Without
this, any authenticated user could reimport another user's harness
config. Also add default-deny for unknown scopes.

M-1: Properly handle malformed JSON in reimport request body instead
of silently discarding parse errors.

H-1/H-2: Gate human-readable printf calls behind !isJSONOutput() in
the CLI update command so --format json produces clean structured
output.
- Use r.Body != nil && r.Body != http.NoBody instead of r.ContentLength
  for chunked/HTTP2 compatibility in reimport handler
- Add nil check for harness config after GetHarnessConfig lookup
- Use cmd.Context() instead of context.Background() in update command
- Add nil checks for resp after Hub List calls in both update functions
- Add nil check for result after Reimport() call to prevent panic on
  (nil, nil) return
- Return error when user-provided projectPath fails to resolve instead
  of silently ignoring it
@ptone ptone force-pushed the scion/harness-journey-p1 branch from 73daa89 to 4fc7f1b Compare June 18, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants