Flatpak

[Jordonbc]

No description provided.

[github-actions]

Here are the findings from reviewing this PR:

---

Important: Fragile sed-based YAML update in publish-stable.yml:221-224

The new "Update Flathub manifest and metadata" step uses a sed range pattern to update tag/commit in the external manifest:

sed -i "/url: https:\/\/github.com\/Open-VCS\/OpenVCS.git/,/dest: \./{
  s|tag: .*|tag: ${RELEASE_TAG}|
  s|commit: .*|commit: ${RELEASE_COMMIT}|
}" io.github.jordonbc.OpenVCS.yml

Risk: If the Flathub manifest's structure changes — e.g., the dest: line is removed or another source block also uses dest: . — the sed range will match the wrong section (or fail to close, applying substitutions from the matching line to EOF). Since the workflow then commits and pushes automatically (line 236), corrupted or silently-stale manifests would land in the Open-VCS/flathub repo. sed-based YAML manipulation is inherently fragile because it has no awareness of YAML structure or indentation.

Fix: Either use yq (YAML-aware CLI) to update the fields precisely, or restructure the manifest source definition so tag/commit are triviable single-line targets (e.g., store them in a separate file that yq or a simple sed can update without range matching). At minimum, add validation (e.g., yq eval or python -c "import yaml; ...") after the sed to verify the manifest is still valid YAML before pushing.

Confidence: Medium

---

Minor: Stale packaging/flatpak/ references in README.md and AGENTS.md

The PR deletes packaging/flatpak/ entirely but does not update:

• README.md:151 — "Flatpak packaging exists under packaging/flatpak/"
• README.md:248 — tree diagram listing packaging/flatpak/
• README.md:312 — reference to packaging/flatpak/
• README.md:325 — dead link [packaging/flatpak/README.md](packaging/flatpak/README.md)
• AGENTS.md:10 — "packaging/flatpak/: Flatpak manifests and Flatpak-specific build notes."

These are now dead references. As the PR moves the manifest to an external repo, these should be updated to point to Open-VCS/flathub (or removed if no longer needed).

Confidence: High

---

Minor: just build-flatpak now requires a sibling ../flathub/ repo clone

The Justfile now references ../flathub/io.github.jordonbc.OpenVCS.yml but no instruction tells developers they must clone Open-VCS/flathub as a sibling directory. The deleted packaging/flatpak/README.md was the only documentation for local flatpak builds. A note in the Justfile or in the project README would close this gap.

Confidence: High

---

No blocking issues were found.

opencode session  |  github run

[github-actions]

Now I have a clear picture of all changes. Here are my review findings:

---

Important: README.md has four stale references to deleted packaging/flatpak/

• README.md:151 — "Flatpak packaging exists under packaging/flatpak/."
• README.md:248 — ├── packaging/flatpak/ in the repository layout tree.
• README.md:312 — packaging/flatpak/ in the Flatpak section.
• README.md:325 — [packaging/flatpak/README.md](packaging/flatpak/README.md) — broken link to a deleted file.

The PR deletes the entire packaging/flatpak/ directory but leaves these references intact, including a broken hyperlink. Users or contributors following these docs will hit 404s or point at nonexistent paths.

High — The link is objectively broken; the section text is now misleading.

---

Important: AGENTS.md:10 references deleted directory

AGENTS.md:10 reads packaging/flatpak/: Flatpak manifests and Flatpak-specific build notes. — both the directory and the referenced README were deleted by this PR. The AGENTS.md guidelines in this repo explicitly say "Keep this AGENTS … current whenever workflows, tooling, or responsibilities change."

High — Docs that the repo's own conventions require to be kept in sync are now stale.

---

Important: publish-stable.yml:233 — set -x echos secret-laden command to logs

The step at line 211-236 activates set -euxo pipefail (line 213), then at line 233 executes:

git remote set-url origin "https://x-access-token:${{ secrets.FLATHUB_REPO_PAT }}@github.com/Open-VCS/flathub.git"

The -x flag prints every command before execution. The secret is embedded directly in the command string. GitHub Actions does mask secrets in logs, but combining set -x with secrets in command-line arguments is a known anti-pattern — masking can fail if the secret is short, appears in a form that doesn't match the exact token string, or is partially truncated by the shell before masking runs.

Medium — The runner is ephemeral and GitHub's masking usually works, but this is the sort of pattern that leads to accidental secret exposure. Use git remote set-url origin "https://github.com/Open-VCS/flathub.git" followed by git config --local http.https://github.com/.extraheader "AUTHORIZATION: bearer ${{ secrets.FLATHUB_REPO_PAT }}" or use gh auth instead of embedding the token in the URL string.

---

Important: publish-stable.yml:221-224 — sed range-based manifest modification is fragile

sed -i "/url: https:\/\/github.com\/Open-VCS\/OpenVCS.git/,/dest: \./{
  s|tag: .*|tag: ${RELEASE_TAG}|
  s|commit: .*|commit: ${RELEASE_COMMIT}|
}" io.github.jordonbc.OpenVCS.yml

The address range /pattern1/,/pattern2/ depends on the YAML layout having exactly one source entry with dest: . between the git URL and the end of the intended source block. If the flathub manifest is restructured (e.g., a non-git source with dest: . is added anywhere in the manifest), the range terminates early and the sed substitutions either apply to the wrong block or fail silently. There is no post-sed validation that tag: or commit: lines were actually changed.

Medium — The current flathub manifest likely works with this pattern, but the pattern has no safeguards against structural changes in the external repo. A follow-up grep check after the sed would be a cheap safety net.

---

Minor: publish-stable.yml:235 — || echo masks commit failures but git push proceeds unconditionally

git commit -m "chore: bump to ${RELEASE_TAG}" || echo 'Nothing to commit'
git push

If the sed modifications produce no actual diff (e.g., the tag/commit were already set), git commit fails with "nothing to commit" and the script continues. That part is fine. However, if the sed modified files but git commit itself fails for another reason (e.g., a pre-commit hook failure), git push still runs the next line and could push unintended, unstaged content from a prior commit on the branch. The git push at line 236 should be behind a check, or the step should stop on commit failure.

Low — Unlikely in practice since flathub repos rarely have pre-commit hooks, but the error handling is subtly wrong.

---

No blocking issues found. No data-loss, build-failure, race-condition, or correctness bugs that would prevent merging.

opencode session  |  github run

[github-actions]

Review Findings

Blocking: Undefined variable $RELEASE_TAG with set -u at publish-stable.yml:223

Line 213 activates set -euxo pipefail, which includes -u (treat unset variables as errors). Line 223 references ${RELEASE_TAG}, but no step or env defines RELEASE_TAG. The shell exits with RELEASE_TAG: unbound variable before git commit runs. The || echo never executes.

The version tag is available as ${{ steps.version.outputs.tag }}.

Impact: The "Update Flathub manifest and metadata" step always fails. The flathub repo is never updated after a release.

Fix: Replace ${RELEASE_TAG} with ${{ steps.version.outputs.tag }}.

Confidence: High

---

Blocking: Git commands operate on the main repo, not the flathub checkout (publish-stable.yml:219-224)

The actions/checkout at line 37-42 checks out the Open-VCS/flathub repo into the flathub/ subdirectory as a separate git repository (with its own .git). The node script at line 214 correctly writes into flathub/, but the git commands at lines 219-224 run from GITHUB_WORKSPACE (the main repo root). This means:

• git remote set-url origin (line 221) changes the main repo's origin to the flathub URL — this is a side-effect that corrupts the main repo's remote config.
• git add io.github.jordonbc.OpenVCS.yml ... (line 222) looks for these files in the main repo's working tree, where they don't exist. git add fails with exit code 128, which under set -e aborts the step.
• Even if git add somehow succeeded, git commit and git push would operate on the main repo, not the flathub repo.

Impact: The flathub manifest update mechanism is entirely non-functional. No files are ever committed or pushed to the flathub repo.

Fix: Add working-directory: flathub to this step, and adjust the node script path accordingly (e.g., node ../scripts/update-flathub.js . ...). Alternatively, keep the step in the root and prepend cd flathub && before each git command.

Confidence: High

---

Important: set -x exposes FLATHUB_REPO_PAT in workflow logs (publish-stable.yml:213,221)

Line 213 activates set -x, which prints every command before execution. Line 221 embeds the PAT directly in the git remote URL string. While GitHub Actions masks secrets, URL-embedded tokens (https://x-access-token:SECRET@github.com/...) are a known masking edge case — the secret may not be recognized in this encoding.

Fix: Use set +x before line 221 and set -x after, or use git config --local http.https://github.com/.extraheader "AUTHORIZATION: bearer ${{ secrets.FLATHUB_REPO_PAT }}" instead of URL embedding.

Confidence: Medium

---

Important: git commit || echo masks non-trivial failures (publish-stable.yml:223-224)

The || echo pattern swallows any git commit failure (hook rejection, permission errors, etc.) and unconditionally proceeds to git push. This could push stale content or produce confusing failures.

Fix: Replace with git diff --staged --quiet || git commit -m "...", which exits 0 when there is nothing to commit but still fails on genuine errors.

Confidence: Low (unlikely in practice, but the error handling is wrong)

---

Minor: updateMetainfo does not deduplicate release entries (scripts/update-flathub.js:66-85)

The function inserts a new <release> element after <releases> every time it runs, without checking whether a release with the same version already exists. If the workflow is re-run for the same version (e.g., after a failed push), duplicate release entries accumulate in the AppStream metadata.

Impact: Cosmetic — AppStream tools may show duplicate entries.

Fix: Check if the version already exists in the XML before inserting, or replace the existing entry.

Confidence: Medium

---

Minor: Duplicate scripts/ entry in README tree diagram (README.md:248-249)

The repository layout tree shows two identical ├── scripts/ entries on consecutive lines. This was introduced by the diff that replaced packaging/flatpak/ with scripts/ without removing the pre-existing scripts/ line.

Impact: Cosmetic.

Fix: Remove the duplicate line.

Confidence: High

opencode session  |  github run

[github-actions]

Blocking: Regex expects repo URL at line start, but it appears after url: — scripts/update-flathub.js:64

The regex in updateManifest is:

`(${escapeRegex(MAIN_REPO_URL)}\\n)(\\s+)tag: .*\\n(\\s+)commit: .*\\n(\\s+)dest: \\.`

This requires https://github.com/Open-VCS/OpenVCS.git to appear at column 0, followed by newline. But in the actual Flathub YAML, the URL appears after url::

      - type: git
        url: https://github.com/Open-VCS/OpenVCS.git
        tag: ...
        commit: ...
        dest: .

The regex never matches, so yaml.replace() returns the string unchanged. The countBefore !== countAfter guard also passes (no tag: lines created or destroyed), making this a silent no-op. The manifest tag and commit are never updated.

Impact: The Flathub repo receives stale tag/commit values on every stable release. The manifest points at whatever the previous commit was, so subsequent Flathub builds rebuild the same revision.

Fix: Change the regex to match the whole url: line instead of just the URL:

`(\\s+url: ${escapeRegex(MAIN_REPO_URL)}\\n)(\\s+)tag: .*\\n(\\s+)commit: .*\\n(\\s+)dest: \\.`

The replacement callback already uses urlLine as-is, so it will preserve the url: prefix.

Confidence: High

---

Important: updateMetainfo assumes <releases> element exists — scripts/update-flathub.js:79

The function searches for <releases>\n to insert a new <release> entry:

xml.replace(/(\s*<releases>\s*\n)/, ...)

The metainfo file deleted from this repo (packaging/flatpak/io.github.jordonbc.OpenVCS.metainfo.xml) does not contain a <releases> element. If the Flathub repo's copy still lacks it, the substitution silently does nothing — no release entry is added, no error is raised.

Impact: AppStream metadata never includes release information. Cosmetic on its own, but means the manifest-update automation does less than advertised.

Fix: Add the <releases> parent element if it doesn't exist, or verify its presence and fail fast.

Confidence: Medium (depends on the flathub repo's current metainfo content, which isn't visible in this PR)

---

Minor: Flatpak CI only runs on workflow_dispatch — .github/workflows/flatpak-test.yml:3

The new flatpak-test.yml has no pull_request or push trigger, only workflow_dispatch. This means Flatpak build regressions won't be caught automatically on PRs. Consider adding pull_request: if CI runner time allows.

---

No blocking issues beyond the regex bug above. The previous review concerns about stale README/AGENTS.md references and working-directory were addressed in later commits (cd flathub, doc updates).

opencode session  |  github run

[github-actions]

Review Findings

Important: update-flathub.js:52-61 — updateManifest no-op when regex doesn't match passes validation silently

The updateManifest function validates the regex substitution by comparing tag: line count before and after:

const countBefore = (yaml.match(/tag: /g) || []).length;
yaml = yaml.replace(/* regex */, /* replacement */);
const countAfter = (yaml.match(/tag: /g) || []).length;
if (countBefore !== countAfter) { process.exit(1); }

When the regex does match, it replaces exactly one tag: line with one tag: line — countBefore === countAfter. When the regex does not match (e.g. the flathub manifest format diverges from what the regex expects), yaml is returned unchanged and countBefore === countAfter still holds. The validation passes, the file is written (same content), and "Bumped tag → …" is logged. The pipeline reports success but the manifest was never updated.

Impact: The flathub repo silently receives stale tag/commit values on every stable release. The error is invisible until someone notices no new flathub builds are appearing.

Fix: Track whether the regex actually produced a match:

let matched = false;
yaml = yaml.replace(regex, (...args) => { matched = true; return replacement; });
if (!matched) {
  console.error(`ERROR: no match found for ${MAIN_REPO_URL} in manifest`);
  process.exit(1);
}

Confidence: High

---

Important: flatpak-test.yml:3 — workflow only triggers on workflow_dispatch, never on PRs

The new flatpak-test.yml has no pull_request: or push: trigger. Changes to this repo that break the flatpak build (new dependencies, build system changes, file path changes, etc.) are never caught automatically. The only detection path is publish-stable.yml which runs at release time.

Impact: Flatpak build regressions land silently and are only discovered during a stable release.

Fix: Add pull_request: (and ideally push: to Dev) so regressions are caught during PR review. For example:

on:
  workflow_dispatch:
  pull_request:
    branches: [ Dev ]

Confidence: High

---

Minor: README.md:318 — refers to nonexistent Client/ directory

The README says "run just build-flatpak from the Client/ directory." No Client/ directory exists in this repository. The Justfile's ../flathub/ reference and the CI's flathub/ subdirectory both indicate the command runs from the repo root.

Fix: Change to "from the repository root" or remove the directory reference.

Confidence: High

---

No blocking issues found. The PR correctly moves the flatpak manifest to the external Open-VCS/flathub repo, updates the pipeline paths, and the cd flathub pattern in publish-stable.yml:219 properly scopes git operations. The only correctness risk is the silent-no-op validator in the update script, and the only coverage gap is the missing PR trigger on the test workflow.

opencode session  |  github run

[github-actions]

Review Findings

No blocking or important issues found in the current code. All concerns raised in earlier review rounds (working-directory scoping, PAT leakage, validation edge cases, stale README/AGENTS.md references, regex correctness) have been addressed in subsequent commits within this 20-commit PR.

Minor: Redundant tag construction in scripts/update-flathub.js:105-107

parseArgs() already returns tag: openvcs-v${version}`` on line 37. Lines 106-107 then destructure version and re-derive `tag = openvcs-v${version}`, overwriting the identical property from the spread on line 111. The behavior is identical, but the re-assignment is dead code.

Remove line 107 and pass args directly (or remove tag from parseArgs return):

-const { flathubDir, version } = args;
-const tag = `openvcs-v${version}`;
+const { flathubDir } = args;

Confidence: High

---

Minor: Redundant \ before / inside character class in scripts/update-flathub.js:86

Line 86 uses (\s*<releases>\s*\n) — the backslash before \n inside the template literal correctly produces a newline in the regex. Not a bug, just noting there is no issue here.

Confidence: High (no-op clarification)

---

Minor: Flatpak CI test workflow has no PR trigger

flatpak-test.yml:3-4 only triggers on workflow_dispatch. The workflow builds whatever released tag the flathub manifest currently points to, not the PR's code, so it validates the build infrastructure rather than the PR's changes. This is an intentional tradeoff — flatpak-builder is resource-intensive. Consider adding pull_request: and updating the manifest locally to point at the PR commit if regression detection for flatpak builds is desired in the future.

Confidence: High (design observation, not a bug)

opencode session  |  github run

[github-actions]

No blocking issues found in the current code. The prior reviews identified multiple issues that have been addressed in subsequent commits within this PR (cd flathub scoping, matched flag in the JS script, documentation updates, permissions block in flatpak-test.yml, fixed regex pattern). One remaining concern:

---

Important: OPENVCS_OFFICIAL_RELEASE not forwarded into flatpak-builder sandbox — publish-stable.yml:126-127

export OPENVCS_OFFICIAL_RELEASE='1'
flatpak-builder --user --force-clean --repo=repo build-flatpak flathub/io.github.jordonbc.OpenVCS.yml

flatpak-builder runs build commands inside a bubblewrap sandbox that does not inherit arbitrary host environment variables. The export makes OPENVCS_OFFICIAL_RELEASE visible to flatpak-builder itself, but not to the cargo build process inside the sandbox, which is where Backend/build.rs:378 reads it:

let official = is_truthy_env("OPENVCS_OFFICIAL_RELEASE") || (head_is_version_tag && !dirty);

Impact: The flatpak stable build artifact likely carries a git-metadata version suffix (e.g., 0.5.0+dev.abcdef) instead of the clean package version (0.5.0). The tauri-apps/tauri-action step at line 165 handles this correctly (GitHub Actions env: context), but the parallel flatpak build path does not.

Fix: Pass the variable via flatpak-builder's --env flag:

flatpak-builder --env=OPENVCS_OFFICIAL_RELEASE=1 --user --force-clean --repo=repo build-flatpak flathub/io.github.jordonbc.OpenVCS.yml

Alternatively, configure build-options.env in the flathub manifest and/or add it to the flatpak-test.yml workflow for parity.

Confidence: High — this is a known property of flatpak-builder/bubblewrap sandboxing; the env-var-escape-through-export assumption is incorrect.

opencode session  |  github run

[github-actions]

Review Findings

Minor: Unused variable tag in scripts/update-flathub.js:107

const { flathubDir, version } = args;
const tag = `openvcs-v${version}`;

parseArgs() already returns { flathubDir, version, commit, tag: \openvcs-v${version}` }. Line 110 passes argsdirectly toupdateManifest, which reads args.tag. The local tag` computed on line 107 is never referenced. Dead code.

Remove line 107 — it has no effect on behaviour.

Confidence: High

---

Minor: flatpak-test.yml:65 does not pass --env=OPENVCS_OFFICIAL_RELEASE=1

The test workflow's flatpak-builder invocation lacks --env=OPENVCS_OFFICIAL_RELEASE=1, while the publish workflow (publish-stable.yml:128) has it. This means the test build may produce a version string with a git-metadata suffix (0.x.y+dev.abcdef), diverging from the publish workflow's output. For a CI test that only validates build success this is benign, but it means the test does not exercise the exact same build configuration as the release path.

Add --env=OPENVCS_OFFICIAL_RELEASE=1 to the flatpak-builder invocation if parity with the publish workflow is desired.

Confidence: High

---

No blocking or important issues found. All previously flagged concerns (regex correctness, cd flathub scoping, PAT leakage via set -x, matched flag validation, stale README/AGENTS.md references, permissions block, OPENVCS_OFFICIAL_RELEASE forwarding) have been addressed in this PR's commit history.

opencode session  |  github run

[github-actions]

Review Findings

No blocking or important issues found. All significant concerns raised in prior reviews (regex correctness, cd flathub git scoping, PAT leakage via set +x/set -x, matched-flag validation in the update script, stale README/AGENTS.md references, OPENVCS_OFFICIAL_RELEASE forwarding via --env, and missing permissions block in flatpak-test.yml) have been addressed in this PR's commit history.

Minor: Unused variable tag in scripts/update-flathub.js:107

const { flathubDir, version } = args;
const tag = `openvcs-v${version}`;  // ← dead code

parseArgs() (line 36) already returns { flathubDir, version, commit, tag: \openvcs-v${version}` }. Line 110 passes argsdirectly toupdateManifest, which reads args.tag. The local tagcomputed on line 107 shadows the property fromargs` but is never referenced. Dead code; no behavioral impact.

Remove line 107.

Confidence: High

---

Minor: No PR trigger on flatpak-test.yml:3

The workflow only triggers on workflow_dispatch. This means Flatpak build regressions (e.g., new dependencies, file path changes, build system changes) are never caught during PR review — only at release time via publish-stable.yml. Given that flatpak-builder is resource-intensive, this is a deliberate tradeoff, but the result is that changes breaking the flatpak build go undetected until a stable release.

Confidence: High

---

Minor: git push proceeds if git commit fails in publish-stable.yml:228

git diff --staged --quiet || git commit -m "chore: bump to openvcs-v${{ steps.version.outputs.version }}"
git push

Under set -e, commands in a || list do not trigger exit-on-error. If git commit fails (e.g., pre-commit hook rejection), git push still runs and pushes whatever was previously committed (typically nothing new since the checkout was fresh). The failure mode is messy but not destructive — git push would succeed with "Everything up-to-date" or fail harmlessly. Already noted in prior reviews.

Confidence: Low (unlikely in practice for a flathub CI checkout)

opencode session  |  github run