Skip to content

Fix hardcoded working-directory: code; add working_dir input - #6

Open
eliran-ops wants to merge 6 commits into
mainfrom
fix/working-dir-autodetect
Open

Fix hardcoded working-directory: code; add working_dir input#6
eliran-ops wants to merge 6 commits into
mainfrom
fix/working-dir-autodetect

Conversation

@eliran-ops

@eliran-ops eliran-ops commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Drop hardcoded working-directory: code from the resolve step. Auto-detect prefers ./code, falls back to .. New working_dir input for explicit override.
  • Harden yq queries via strenv(); add services[].path as the dockerfile fallback (existing test asserted it; implementation was missing).
  • Fix stale test-skyhook-config-overrides-inputs — asserted the reversed contract since PR Fix input contract: user-provided context/dockerfile override service_name resolution #5; renamed and flipped.
  • Add 3 integration tests + fixture (test/fixtures/code-checkout/) covering auto-detect, explicit working_dir, trailing-slash tolerance.

Why

CI on main has been red since PR #5 (5 days). Every service_name test fails with bash: working directory '.../code' not found. The action's own repo doesn't check into code/, and neither does any non-Skyhook consumer. @v1 floating tag inherits the breakage.

No Linear issue — this is an in-session ad-hoc fix off origin/main.

Where to start reading

  • action.yml:14-17 — new working_dir input.
  • action.yml:199-305 — rewritten resolve step (auto-detect + normalize_path / join_path helpers + strenv()).
  • .github/workflows/test.yml:269-339 — three new jobs covering the new code paths.

Risky vs mechanical

Risky — eyes here:

  • Auto-detect order: code first, then .. A consumer with both ./code/.skyhook/skyhook.yaml and ./.skyhook/skyhook.yaml silently picks code/. test-skyhook-autodetect-code exercises exactly this scenario and verifies the choice.
  • Behavior change: previously the action probed only code/.skyhook/skyhook.yaml. Now non-Skyhook consumers (yaml at workspace root) succeed. If anyone relied on the action failing in that case, they'll see a behavior change.
  • services[].path dockerfile fallback: existing test-skyhook-path-fallback asserted this but main never implemented it (test was passing under prior different bug). This PR actually makes it work — anyone whose Dockerfile lived elsewhere and depended on the silent Dockerfile workspace-root fallback could see a diff.

Mechanical:

Notes

  • After approval, recommend retagging @v1 (currently 1fdb7ba) so existing consumers pick the fix.
  • 6 commits split logically. Squash-on-merge if preferred — no --amend per CLAUDE.md.

🤖 Generated with Claude Code


Note

Medium Risk
Changes how the action locates and resolves .skyhook/skyhook.yaml and derived context/dockerfile, which can affect builds for repos with multiple candidate config locations or relying on previous fallback behavior.

Overview
Fixes service_name config resolution by removing the hardcoded working-directory: code and adding a new working_dir input; when unset, the resolver now auto-detects .skyhook/skyhook.yaml by preferring ./code and falling back to ..

Tightens and extends resolution behavior: yq lookups now use strenv() for safer querying, path joining/normalization is centralized, and services[].path is implemented as an additional Dockerfile fallback when buildTool.docker.* isn’t set.

Updates tests and docs: flips/renames the workflow test to enforce the “explicit context/dockerfile inputs override config” contract, adds three integration jobs covering auto-detect and working_dir (including trailing slashes), and documents service_name/working_dir behavior in README.md.

Reviewed by Cursor Bugbot for commit e94945f. Bugbot is set up for automated code reviews on this repo. Configure here.

eliran-mic and others added 6 commits May 5, 2026 14:15
The resolve step pinned `working-directory: code` and `REPO_PREFIX=code`,
which assumed every consumer's checkout lives at `./code`. The action's
own CI repo doesn't, so every `service_name` test on main has been red
since PR #5 (`bash: working directory '.../code' not found`). Any
non-Skyhook consumer hits the same wall.

Changes:
- New `working_dir` input. Empty (default) auto-detects: probe `./code`
  first (Skyhook convention), fall back to `.`. Resolved paths emit
  relative to whichever wins, so existing consumers see no change.
- Drop `working-directory: code` from the resolve step. Caller inputs
  routed through `env:` so service_name / context / dockerfile can't
  inject into the bash via `${{ }}` substitution.
- Switch yq queries to `strenv(SERVICE_NAME)` — quote-safe lookup.
- Tolerate `working_dir: code/` (strip trailing slash) and `contextPath: .`
  / `./` (normalize to empty) so paths stay clean.
- Add `services[].path` as the dockerfile fallback when neither
  contextPath nor dockerfilePath is set; closes the gap that left
  `test-skyhook-path-fallback` asserting unimplemented behavior.
- Rename `test-skyhook-config-overrides-inputs` →
  `test-skyhook-inputs-override-config` and flip its assertion to
  match PR #5's "inputs win" contract (was previously asserting the
  pre-PR#5 reversed semantic).
- Add three integration jobs for the new code paths: auto-detect picking
  `./code`, explicit `working_dir`, and trailing-slash tolerance. Backed
  by a `test/fixtures/code-checkout/` tree with a unique marker so the
  grep distinguishes prefix choices.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
yq's `.[0].name // ""` already coerces every reachable null path
(missing service, missing name field, missing services key) to "".
The literal-string `null` branch was unreachable and confusing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single-row addition to Build Configuration. Mentions service_name
parenthetically since working_dir only applies in that mode, but
does not attempt to document service_name itself — that's a
pre-existing gap from PR #2 and out of this PR's scope.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
action.yml: strip leading `./` and any number of trailing slashes from
`working_dir` so `code`, `code/`, `code//`, `./code`, `./code/` all
resolve to the same prefix and emit clean output paths. `.` stays
as `.` (explicit "workspace root" — distinct from empty/auto-detect).

README.md: add a `service_name` row to the inputs table. Pre-existing
gap from PR #2, but the `working_dir` row added in this branch
references service_name and would be confusing without it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- service_name now states the inputs-win rule (symmetric with the
  context/dockerfile descriptions which already document it).
- working_dir says "used as the prefix" instead of "emitted relative
  to this dir" — outputs are `<working_dir>/<sub>`, so absolute
  working_dir produces absolute output paths. Previous wording was
  misleading.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Other empty-default rows use `-`; `''` was visually inconsistent.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@eliran-ops
eliran-ops requested a review from hisco May 5, 2026 13:05
@eliran-ops

Copy link
Copy Markdown
Contributor Author

@hisco i can merge, but i'd like a review

Comment thread action.yml
description: 'Service name to build, as defined in .skyhook/skyhook.yaml (services[].name)'
description: 'Service name to build, as defined in .skyhook/skyhook.yaml (services[].name). Inputs `context` / `dockerfile` win over values resolved from this name.'
required: false
working_dir:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This input is the only concept that should stay... the rest of the changes are not relvant.

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.

3 participants