-
Notifications
You must be signed in to change notification settings - Fork 2
refactor(action): simplify v2 flow to CLI incremental/full contract #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f61339f
d59229f
4bc6cf3
220cc37
de457c0
e13086e
060e7e3
6397805
a7259e0
96de6a7
812ed52
bde08fa
fa74487
463ab43
07de887
b6d04c3
3024fdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,7 @@ jobs: | |
| permissions: | ||
| contents: write # push the generated baseline branch | ||
| pull-requests: write # workflow_dispatch may exercise pull_request delivery | ||
| id-token: write # mint per-request OIDC credentials for the relay | ||
| steps: | ||
| # Dogfood: run the action from the checked-out repo (uses: ./) so pushes to | ||
| # main exercise the action code on main, not the last published release. | ||
|
|
@@ -145,15 +146,9 @@ jobs: | |
| - uses: ./ | ||
| with: | ||
| mode: sync | ||
| force_full: ${{ inputs.force_full || false }} | ||
| # Push events retain direct delivery to their branch. A manual | ||
| # pull_request-strategy run targets main even though the workflow code | ||
| # itself is checked out from the feature ref being dogfooded. | ||
| target_branch: ${{ github.event_name == 'workflow_dispatch' && inputs.sync_strategy == 'pull_request' && 'main' || github.ref_name }} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still need to support this no? I am a bit puzzled what was this line doing and what is the behavior now?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line made a manually dispatched rolling-PR sync target |
||
| sync_strategy: ${{ inputs.sync_strategy || 'push' }} | ||
| sync_pr_branch: ${{ inputs.sync_pr_branch || 'codeboarding/sync' }} | ||
| # App token authenticates the baseline push so the commit is attributed | ||
| # to the CodeBoarding App (logo avatar). Falls back to the workflow token, | ||
| # which can push because this job grants contents: write. | ||
| push_token: ${{ steps.codeboarding-app-token-client.outputs.token || steps.codeboarding-app-token-app.outputs.token || github.token }} | ||
| llm_api_key: ${{ secrets.OPENROUTER_API_KEY }} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we dropping the operouter key here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That line made this repository’s dogfood sync use its own OpenRouter key directly. Removing it switched dogfood to the hosted OIDC tier because the job grants |
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| #!/usr/bin/env python3 | ||
| """Thin helper to execute CodeBoarding CLI incremental/full commands. | ||
|
|
||
| The action is intentionally logic-light: all analysis orchestration happens in | ||
| shell through this script's small JSON contract parser, which only invokes | ||
| CodeBoarding's own ``incremental`` and ``full`` commands. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import json | ||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| PROG = "codeboarding" | ||
|
|
||
|
|
||
| class AnalysisError(RuntimeError): | ||
| pass | ||
|
|
||
|
|
||
| def _parse_bool(value: object, *, field: str) -> bool: | ||
| if isinstance(value, bool): | ||
| return value | ||
| if isinstance(value, str): | ||
| lowered = value.strip().lower() | ||
| if lowered in {"true", "1", "yes", "y"}: | ||
| return True | ||
| if lowered in {"false", "0", "no", "n"}: | ||
| return False | ||
| raise AnalysisError(f"Invalid contract field '{field}': {value!r}") | ||
|
|
||
|
|
||
| def _normalize_analysis_path(payload: dict, output_dir: str) -> Path: | ||
| path = payload.get("analysis_path") | ||
| if not isinstance(path, str) or not path.strip(): | ||
| raise AnalysisError("Missing or empty 'analysis_path' in CLI response") | ||
|
|
||
| candidate = Path(path) | ||
| if not candidate.is_absolute(): | ||
| candidate = Path(output_dir) / candidate | ||
| return candidate | ||
|
Comment on lines
+41
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the CLI returns a relative path such as Useful? React with 👍 / 👎. |
||
|
|
||
|
|
||
| def _parse_cli_response(raw: str, output_dir: str) -> tuple[bool, Path | None, dict]: | ||
| if not raw.strip(): | ||
| raise AnalysisError("CodeBoarding command produced no JSON output") | ||
|
|
||
| try: | ||
| payload = json.loads(raw) | ||
| except json.JSONDecodeError as exc: | ||
| payload = None | ||
| lines = raw.splitlines() | ||
| for index, line in enumerate(lines): | ||
| if line.lstrip().startswith("{"): | ||
| try: | ||
| payload = json.loads("\n".join(lines[index:])) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if payload is None: | ||
| raise AnalysisError(f"Invalid CodeBoarding JSON response: {exc}") from exc | ||
|
|
||
| if not isinstance(payload, dict): | ||
| raise AnalysisError("CodeBoarding JSON response is not an object") | ||
|
|
||
| requires_full = _parse_bool(payload.get("requiresFullAnalysis"), field="requiresFullAnalysis") | ||
| if requires_full and not payload.get("analysis_path"): | ||
| return True, None, payload | ||
|
|
||
| analysis_path = _normalize_analysis_path(payload, output_dir) | ||
|
|
||
| if not analysis_path.is_file(): | ||
| raise AnalysisError(f"analysis_path points to a non-file: {analysis_path}") | ||
|
|
||
| return requires_full, analysis_path, payload | ||
|
|
||
|
|
||
| def _run_command(args: list[str], output_dir: Path) -> str: | ||
| process = subprocess.Popen( | ||
| args, | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| bufsize=1, | ||
| cwd=str(output_dir.parent), | ||
| env=None, | ||
| ) | ||
| if process.stdout is None: # pragma: no cover - guaranteed by stdout=PIPE | ||
| raise AnalysisError(f"Unable to read command output ({' '.join(args)})") | ||
|
|
||
| stdout_lines: list[str] = [] | ||
| for line in process.stdout: | ||
| stdout_lines.append(line) | ||
| # The shell captures this helper's stdout as its result contract. Mirror | ||
| # CLI stdout to stderr so engine progress remains visible in Actions. | ||
| print(line, end="", file=sys.stderr, flush=True) | ||
|
|
||
| return_code = process.wait() | ||
| stdout = "".join(stdout_lines) | ||
| if return_code != 0: | ||
| details = stdout.strip() or f"exit code {return_code}; see command logs above" | ||
| raise AnalysisError(f"Command failed ({' '.join(args)}): {details}") | ||
|
|
||
| return stdout | ||
|
|
||
|
|
||
| def run_incremental(checkout: Path, output_dir: Path) -> tuple[bool, Path | None, dict]: | ||
| output_dir.mkdir(parents=True, exist_ok=True) | ||
| raw = _run_command([PROG, "incremental", "--local", str(checkout), "--output-dir", str(output_dir)], output_dir) | ||
| return _parse_cli_response(raw, str(output_dir)) | ||
|
|
||
|
|
||
| def run_full(checkout: Path, output_dir: Path, depth_level: str) -> Path: | ||
| output_dir.mkdir(parents=True, exist_ok=True) | ||
| _run_command( | ||
| [ | ||
| PROG, | ||
| "full", | ||
| "--local", | ||
| str(checkout), | ||
| "--output-dir", | ||
| str(output_dir), | ||
| "--depth-level", | ||
| str(depth_level), | ||
| "--force", | ||
| ], | ||
| output_dir, | ||
| ) | ||
| analysis_path = output_dir / "analysis.json" | ||
| if not analysis_path.is_file(): | ||
| raise AnalysisError(f"Full analysis did not produce: {analysis_path}") | ||
| return analysis_path | ||
|
|
||
|
|
||
| def main(argv: list[str] | None = None) -> int: | ||
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument("mode", choices=["incremental", "full"], help="Which CLI command to invoke") | ||
| parser.add_argument("--checkout", required=True, help="Path to repository checkout") | ||
| parser.add_argument("--output-dir", required=True, help="Action-owned output directory") | ||
| parser.add_argument("--depth-level", help="Depth passed to full analyses") | ||
|
|
||
| args = parser.parse_args(argv) | ||
| checkout = Path(args.checkout) | ||
| output_dir = Path(args.output_dir) | ||
| if not checkout.is_dir(): | ||
| raise SystemExit(f"Missing checkout directory: {checkout}") | ||
|
|
||
| if args.mode == "incremental": | ||
| requires_full, analysis_path, _ = run_incremental(checkout, output_dir) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we actually setting the sys variable so we know that this run is done via "github_action" and not just a "core"/"oss" call?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No—the wrapper currently invokes the generic |
||
| print(f"analysis_mode=incremental") | ||
| print(f"requires_full_analysis={str(requires_full).lower()}") | ||
| print(f"analysis_path={analysis_path or ''}") | ||
| return 0 | ||
|
|
||
| if not args.depth_level: | ||
| raise SystemExit("--depth-level is required for mode=full") | ||
| analysis_path = run_full(checkout, output_dir, args.depth_level) | ||
| print(f"analysis_mode=full") | ||
| print("requires_full_analysis=false") | ||
| print(f"analysis_path={analysis_path}") | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| try: | ||
| raise SystemExit(main()) | ||
| except AnalysisError as exc: | ||
| print(f"::error::{exc}", file=sys.stderr) | ||
| raise SystemExit(1) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a breaking API change or not? This is important as I will need to push major version if it is the case when releasing this to v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Removing declared inputs or changing their behavior is a breaking public API change. If those removals are intentional, this must ship as
v2; the movingv1tag must remain on the old contract. Keeping a couple of deprecated no-op inputs does not make the overall change backward-compatible.