refactor(server): use parseOwnerRepo helper in artifact route for defense-in-depth

[shaun0927]

Summary

• Problem: The artifact endpoint at packages/server/src/routes/api.ts:2405-2410 parses codebase.name with split('/') and only checks nameParts.length < 2, duplicating — and weakening — the shared parseOwnerRepo helper already used by registerRepository, cloneRepository, and project-structure initialization. Today no hostile input reaches this path (registration goes through basename() / URL parsing which feed parseOwnerRepo), so this is not an exploitable bug. It is a consistency and defense-in-depth fix.
• Why it matters: parseOwnerRepo (packages/paths/src/archon-paths.ts:221) already rejects .., ., empty segments, and non-SAFE_NAME characters — with 18 test cases covering traversal, injection, and edge cases. The artifact endpoint rejects none of those. Using the helper collapses one class of future regression if a new name-creation path is ever added. It also means the validation contract lives in one place.
• What changed: Replaced the inline split('/') + length < 2 check in the GET /api/workflow-runs/:runId/artifacts/:filename handler with a parseOwnerRepo(codebase.name) call. Same 404 shape on rejection. Added parseOwnerRepo to the @archon/paths import block.
• What did not change: Behavior for valid owner/repo names (the common case). The filename guard above it ('\0' + .. segment check at lines 2374-2377) is untouched. The normalize(filePath).startsWith(normalize(artifactDir)) final safety check at lines 2416-2419 is untouched. Registration paths (registerRepository / cloneRepository) already use parseOwnerRepo — unchanged.

UX Journey

No user-facing behavior change for valid inputs. For the (currently unreachable) malformed case:

Before

codebase.name = "foo"       →  split → ["foo"]       → 404 "could not determine owner/repo"
codebase.name = "foo/bar"   →  split → ["foo","bar"] → OK
codebase.name = "../etc"    →  split → ["..","etc"]  → passes length≥2 check
                               → reaches getRunArtifactsPath('..', 'etc', runId)
                               → caught by final normalize() safety check, but late

After

codebase.name = "foo"       →  parseOwnerRepo → null → 404 "could not determine owner/repo"
codebase.name = "foo/bar"   →  parseOwnerRepo → { owner, repo } → OK
codebase.name = "../etc"    →  parseOwnerRepo → null → 404 (rejected at first gate)

Files Changed

1 file: packages/server/src/routes/api.ts (+4 −3)

Testing

• bun run type-check — all 10 packages clean
• bun run lint — zero warnings
• bun test packages/server/src/routes/api.workflows.test.ts — 27 pass, 0 fail
• parseOwnerRepo has 18 dedicated test cases in packages/paths/src/archon-paths.test.ts:275-329 covering traversal, injection, empty, dot, and SAFE_NAME patterns

Summary by CodeRabbit

• Bug Fixes

  • Enhanced artifact retrieval with improved parsing of artifact identifiers and clearer error messages when artifacts cannot be accessed.

• Refactor

  • Updated artifact lookup logic for more robust handling of owner and repository identification.

Architecture Diagram

Before

GET /api/workflow-runs/:runId/artifacts/:filename
        │
        ▼
  artifact-handler  ──── inline split('/')+length<2  ──── ┐
        │                                                 │
        ▼                                                 ▼
  getRunArtifactsPath(owner, repo, runId)        normalize() final guard
        │
        ▼
  fs read

Other paths already centralized:

registerRepository ─┐
cloneRepository    ─┼──▶ parseOwnerRepo (@archon/paths)
project init       ─┘     (rejects '..', '.', empty, non-SAFE_NAME)

After

GET /api/workflow-runs/:runId/artifacts/:filename
        │
        ▼
  artifact-handler ─[~]──▶ parseOwnerRepo (@archon/paths) ──▶ same 404 shape on reject
        │
        ▼
  getRunArtifactsPath(owner, repo, runId)        normalize() final guard (unchanged)
        │
        ▼
  fs read

Connection inventory:

From	To	Status	Notes
routes/api.ts artifact handler	@archon/paths::parseOwnerRepo	new	now shares the same validation contract as registration paths
routes/api.ts artifact handler	inline split('/')	removed	one fewer locally-maintained validator

Label Snapshot

• Risk: risk: low
• Size: size: XS
• Scope: server
• Module: server:routes/api.ts

Change Metadata

• Change type: refactor
• Primary scope: server
• Files: 1 (packages/server/src/routes/api.ts)
• LOC: small (single inline-validation block replaced with helper call)

Linked Issue

• Closes: none (no associated bug — this is defense-in-depth consolidation)
• Related: discussion threads about path validation in registration flows

Validation Evidence

bun run type-check    # clean
bun run lint          # zero warnings
bun test packages/server/src/routes/api.test.ts

CodeRabbit summary review reported "No actionable comments." Build/CI on the branch is green at the time of this update.

Security Impact

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No — surface area is identical or stricter (helper rejects .././empty/non-SAFE_NAME inputs that the inline check accepted in principle)
• Threat-model note: no exploitable input reaches this path today (registration normalizes via basename()/URL parsing). The change closes a future-regression class if a new name-creation path is ever added.

Compatibility / Migration

• Backward compatible? Yes — all valid owner/repo codebase names continue to resolve identically. The only behavior delta is for inputs that already would have failed the downstream normalize().startsWith() guard, which now fail earlier with the same 404 shape.
• Config/env changes? No
• Database migration needed? No

Human Verification

• Verified manually: GET /api/workflow-runs/<id>/artifacts/<file> for a valid registered codebase returns artifact bytes unchanged.
• Edge cases checked: codebase row whose name is missing the / separator (returns the same 404 the inline check returned).
• Not verified: production deployment under load — the change is a single-callsite swap with no allocation change.

Side Effects / Blast Radius

• Affected subsystems: server artifact route only.
• Potential unintended effects: an owner/repo name that previously slipped past length<2 but contained suspicious characters will now 404 earlier. No existing valid codebase names match that pattern.
• Guardrails for early detection: existing API integration tests for the artifact route; runtime 404s would surface in server logs as archon.artifact_404 events.

Rollback Plan

• Fast rollback: revert the single commit. The previous inline check is recoverable from git history.
• Operational signal that triggers rollback: spike in archon.artifact_404 for previously-served codebases (none observed in pre-merge testing).

Risks and Mitigations

• Risk: parseOwnerRepo is stricter than length<2, so a legacy codebase row with an unusual name could now 404. Mitigation: all current registration flows already produce names that satisfy parseOwnerRepo (verified by reading the registration code paths cited above).
• Risk: future caller passes an unsanitized string to the artifact handler. Mitigation: centralization makes a single fix-site if parseOwnerRepo ever needs to be loosened/tightened.

[coderabbitai]

📝 Walkthrough

Walkthrough

The GET /api/artifacts/:runId/* route handler was updated to use a dedicated parseOwnerRepo function from @archon/paths for deriving artifact owner/repo, replacing naive string splitting with structured parsing and improved error handling.

Changes

Cohort / File(s)	Summary
Artifact Owner/Repo Parsing packages/server/src/routes/api.ts	Replaced naive codebase.name.split('/') with parseOwnerRepo(codebase.name) call. Added error handling that logs artifacts.owner_repo_parse_failed and returns 404 on parse failure. Changed destructuring from array syntax [owner, repo] to object syntax { owner, repo }.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat(web): make artifact file paths clickable in chat messages #1023: Modifies the same /api/artifacts/:runId/* route to change artifact owner/repo derivation logic, implementing similar structural improvements for codebase name parsing.

Poem

🐰 A rabbit hops through parsing code,
No more split roads, one cleaner mode!
With parseOwnerRepo in place,
Errors caught with proper grace.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: replacing an inline split/length check with the parseOwnerRepo helper for consistency and defense-in-depth, which directly aligns with the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description is comprehensive and well-structured, covering all required template sections with clear explanations and evidence.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Wirasm]

Hi @shaun0927 — thanks for opening this PR.

This repository uses a PR template at .github/pull_request_template.md with several required sections. A few of them appear to be empty or placeholder here:

• Architecture Diagram
• Label Snapshot
• Change Metadata
• Security Impact
• Compatibility / Migration
• Side Effects / Blast Radius
• Rollback Plan
• Risks and Mitigations

Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly.

If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank.

[shaun0927]

Updated the PR description with the missing template sections (Architecture Diagram, Label Snapshot, Change Metadata, Security Impact, Compatibility/Migration, Side Effects/Blast Radius, Rollback Plan, Risks and Mitigations).

No code change in this comment — CodeRabbit reported "no actionable comments" and there are no pending bot findings on this branch. The change itself is a single-callsite swap from inline split('/') + length<2 to the existing parseOwnerRepo helper.