Open
Conversation
…er instead of a single json file
Contributor
📝 WalkthroughWalkthroughThis PR refactors the Autolab client to support multi-server configurations. It replaces single-server credential management with a directory-based server configuration approach, updates core client APIs to accept Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can use Clang for C/C++ static analysis and code quality checks.Clang provides comprehensive static analysis for C and C++ code, including syntax checking, type checking, and various warning diagnostics. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (12)
src/app_credentials_andrew.h (1)
35-36: Replace placeholder crypto key and IV with guidance.The placeholder values
"EnterYourVerySecretCryptoKeyHere"and"YourVerySecretIV"are helpful reminders, but consider adding a compile-time check or static_assert to prevent accidental use of these defaults in production.src/cmd/cmdimp.h (1)
4-9: Add explicit include forAutolab::ServerInfo.
Autolab::ServerInfois used in the function signature on line 9, butautolab/multi_server.his not directly included. While the code compiles via transitive includes throughclient.h, relying on transitive includes is fragile and can break with refactoring.♻️ Proposed fix
`#include` "autolab/client.h" +#include "autolab/multi_server.h" `#include` "cmdargs.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cmdimp.h` around lines 4 - 9, The header declares Autolab::ServerInfo in the perform_device_flow signature but doesn't include its defining header; add an explicit include for the header that defines Autolab::ServerInfo (e.g. include "autolab/multi_server.h") at the top of the file so the symbol is resolved directly rather than relying on transitive includes; update the includes near init_autolab_client and perform_device_flow declarations accordingly.include/autolab/client.h (1)
35-36: Inconsistent parameter naming:server_info__uses trailing underscores.The parameter
server_info__on line 35 uses double trailing underscores, which is unconventional. Other methods in this file useserver_infowithout the trailing underscores. Consider using consistent naming.📝 Proposed fix
- void get_user_info(User &user, const ServerInfo& server_info__); + void get_user_info(User &user, const ServerInfo& server_info);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/autolab/client.h` around lines 35 - 36, The parameter name server_info__ in the declaration of get_user_info(User &user, const ServerInfo& server_info__) is inconsistent; rename the parameter to server_info for consistency with other methods (e.g., use const ServerInfo& server_info) and update the corresponding definition/implementation and any call sites that use server_info__ to use server_info instead so all references to ServerInfo use the same identifier.README.md (1)
39-44: Minor style improvement.Per static analysis hint: "Inside of" is redundant; consider using "Inside" instead on line 39.
📝 Proposed fix
-Inside of this directory, insert files that follow the format as can be seen in +Inside this directory, insert files that follow the format as can be seen in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 39 - 44, Update the phrasing in README.md where the sentence begins "Inside of this directory..." to remove the redundant "of" so it reads "Inside this directory, insert files that follow the format as can be seen in `lib/server_info_example.txt`"—locate the sentence in the README and replace "Inside of" with "Inside" to satisfy the static analysis style hint.lib/autolab/raw_client.cpp (2)
12-13: Relative include path may be fragile.The include
#include "../../src/context_manager/context_manager.h"uses a relative path that depends on the build directory structure. This could break if the project structure changes or if the library is used from a different location.Consider adding the context_manager directory to the include paths in CMake, or moving the shared header to a common include location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/autolab/raw_client.cpp` around lines 12 - 13, The relative include "#include \"../../src/context_manager/context_manager.h\"" is fragile; replace it with a stable include and/or add the header's parent directory to the build include paths: update the CMake target that builds lib/autolab/raw_client.cpp (or the library target that owns it) to add target_include_directories(...) pointing to the directory that contains context_manager/context_manager.h, then change the source to include it as "#include <context_manager/context_manager.h>" (or move the header to a common include dir and adjust includes accordingly) so the compile no longer depends on "../../" relative paths.
154-160: Consider updating deprecated CURL APIs.Per pipeline warning:
curl_formadd,CURLFORM_COPYNAME,CURLFORM_FILE,CURLOPT_HTTPPOST, andcurl_formfreeare deprecated. Consider migrating to thecurl_mime_*API equivalents to silence deprecation warnings and ensure future compatibility.This is not blocking, but worth tracking for a future update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/autolab/raw_client.cpp` around lines 154 - 160, The code uses deprecated form APIs (curl_formadd, CURLFORM_COPYNAME, CURLFORM_FILE, CURLOPT_HTTPPOST and curl_formfree); replace this with the curl_mime API by creating a mime handle via curl_mime_init(curl), add a part with curl_mime_name(mimepart, "submission[file]") and attach the file using curl_mime_filedata(mimepart, rstate->upload_filename.c_str()), then set the mime on the easy handle with curl_easy_setopt(curl, CURLOPT_MIMEPOST, mime) and free the mime with curl_mime_free(mime) when done; update references in this file from the old symbols (curl_formadd/lastptr/CURLFORM_*/CURLOPT_HTTPPOST/curl_formfree) to the corresponding curl_mime_* calls to silence deprecation warnings and ensure compatibility.include/autolab/multi_server.h (2)
1-2: Header guard uses reserved identifier.Identifiers starting with an underscore followed by an uppercase letter are reserved by the C++ standard for implementation use. Consider using a non-reserved pattern.
Suggested fix
-#ifndef _MULTI_SERVER_H_ -#define _MULTI_SERVER_H_ +#ifndef AUTOLAB_MULTI_SERVER_H +#define AUTOLAB_MULTI_SERVER_HAnd at the end:
-#endif /* _MULTI_SERVER_H_ */ +#endif /* AUTOLAB_MULTI_SERVER_H */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/autolab/multi_server.h` around lines 1 - 2, The header guard _MULTI_SERVER_H_ uses a reserved identifier (leading underscore + uppercase); replace it with a non-reserved macro throughout the file (update both the `#ifndef` and `#define` that reference _MULTI_SERVER_H_ and the corresponding `#endif` comment if present) — for example use MULTI_SERVER_H or a project-scoped name like AUTOLAB_MULTI_SERVER_H_INCLUDED, ensuring all references in include/autolab/multi_server.h match the new symbol.
18-20: Publicm_server_listexposes internal state and sensitive data.
m_server_listcontainsclient_secretfor each server. Making this public allows external code to directly access and modify sensitive credentials. Consider making this private with controlled access through getter methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/autolab/multi_server.h` around lines 18 - 20, AllServers currently exposes sensitive credentials via the public member m_server_list (vector<ServerInfo>), so make m_server_list private and provide controlled accessors: add a const getter like get_servers() that returns either a const reference or a sanitized copy (omitting client_secret) and a separate method get_server_secret(id) or fetch_secret(id) to retrieve secrets under controlled checks; update any code referencing AllServers::m_server_list to use these methods and keep mutation confined to methods like add_server(...) / update_server(...) on the AllServers class to validate inputs and avoid direct external modification.include/autolab/raw_client.h (1)
73-74: Inconsistent parameter naming:server_info__vsserver_info.Some methods use
server_info__(double underscore suffix) while others useserver_info. This inconsistency reduces readability. Choose one naming convention and apply it consistently.Examples of inconsistency
- Line 73:
server_info__- Line 74:
server_info- Line 127-129:
server_info__- Line 136:
server_info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/autolab/raw_client.h` around lines 73 - 74, The parameter name for ServerInfo is inconsistent (some declarations use server_info__ while others use server_info); standardize to a single name (recommend server_info) across the header and corresponding implementation files by renaming the parameter in all function declarations and matching definitions (e.g., get_user_info(rapidjson::Document &result, const ServerInfo& server_info), get_courses(...), and the functions referenced around the other declarations) so signatures and parameter names are consistent throughout the codebase.lib/autolab/multi_server.cpp (3)
96-96: Signed/unsigned comparison: loop variableintcompared withunsigned.
courses_value.Size()returnsrapidjson::SizeType(typicallyunsigned). Usingintforcourse_idxcreates a signed/unsigned comparison warning and potential issues with large arrays.Suggested fix
- for (int course_idx = 0; course_idx < courses_value.Size(); course_idx++) { + for (rapidjson::SizeType course_idx = 0; course_idx < courses_value.Size(); course_idx++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/autolab/multi_server.cpp` at line 96, The for-loop uses a signed int `course_idx` compared against `courses_value.Size()` (an unsigned rapidjson::SizeType), causing a signed/unsigned warning; change the loop index to an unsigned type such as `rapidjson::SizeType` (or `decltype(courses_value.Size())` / `std::size_t`) in the loop initializing `course_idx` to eliminate the signed/unsigned mismatch when iterating over `courses_value`.
60-60:DT_REGmay not be portable across all POSIX systems.
d_typeandDT_REGare not guaranteed by POSIX and may beDT_UNKNOWNon some filesystems. Consider usingstat()as a fallback to determine if the entry is a regular file whend_type == DT_UNKNOWN.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/autolab/multi_server.cpp` at line 60, The current check using entry->d_type != DT_REG is not portable; update the directory iteration (the readdir loop that accesses entry->d_type and entry->d_name) to handle DT_UNKNOWN by falling back to stat/lstat: if entry->d_type == DT_REG accept as regular file, if entry->d_type == DT_UNKNOWN construct the full pathname (combine the directory path and entry->d_name) and call stat() (or lstat() if symlink behavior is desired) and use S_ISREG(st.st_mode) to decide whether to continue; only skip non-regular files when both the d_type check and the stat fallback indicate the entry is not a regular file.
183-191: Inconsistent exception types for similar failure modes.
get_access_token_from_serverthrowsstd::runtime_error("fatal")while similar lookup failures inget_server_from_courseandget_server_from_namethrowInvalidInputException. Consider using a consistent exception type for authentication/lookup failures.Suggested fix
Logger::fatal << "Could not find access token. Please rerun autolab setup for this course." << Logger::endl; - throw std::runtime_error("fatal"); + throw InvalidInputException {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/autolab/multi_server.cpp` around lines 183 - 191, Replace the inconsistent std::runtime_error thrown in AllAuthInfo::get_access_token_from_server with the same InvalidInputException used by get_server_from_course and get_server_from_name: after failing to find a matching AuthInfo, construct and throw an InvalidInputException with a clear message (e.g., "Could not find access token for server '<server_name>'; please rerun autolab setup for this course") and ensure any logging remains consistent with other lookup failures (remove or align Logger::fatal if those functions don't use it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/autolab/multi_server.h`:
- Around line 27-29: The comment for get_server_from_course (and similarly
get_server_from_name) is incorrect: these functions return ServerInfo and, per
the implementation in multi_server.cpp, throw an exception when the requested
server/course is not found; update the comment to state that they return a
ServerInfo on success and throw an exception (specify exception type if known)
when no matching course/server exists so the declaration matches the actual
behavior.
- Around line 46-48: Update the misleading comment above the token accessors:
change the comment for get_access_token_from_server to state it throws on
failure (instead of returning ""), and update the comment for
get_refresh_token_from_server to explicitly state it returns an empty string
when not found; reference the functions get_access_token_from_server(const
std::string& server_name) and get_refresh_token_from_server(const std::string&
server_name) so callers know the different error behaviors.
In `@include/autolab/raw_client.h`:
- Line 135: Rename the misleading parameter name server_name to server_info in
the declaration of get_token_from_authorization_code(std::string
authorization_code, const ServerInfo& server_info) and update the corresponding
definition/implementation and all call sites to use server_info; ensure any
documentation/comments and variable usages inside the
get_token_from_authorization_code function that referenced server_name are
updated to the new identifier to match the ServerInfo type.
In `@lib/all_servers_dirname.h.template`:
- Around line 1-10: all_servers_dirname is a global std::string that can be used
before it's initialized by g_all_servers in another TU, causing static
initialization order problems; replace the global variable with an accessor
function (e.g., get_all_servers_dirname()) that returns a const std::string&
initialized as a local static (lazy init) and update all call sites (including
the g_all_servers construction in multi_server.cpp) to use
get_all_servers_dirname() instead of the global variable to ensure safe
initialization order.
- Around line 6-8: The header currently defines the variable all_servers_dirname
which causes an ODR violation; change the header to only declare the symbol
(e.g., extern std::string all_servers_dirname;) or mark it inline (C++17) to
allow header definitions, and move the single definitive definition into a .cpp
translation unit (e.g., define std::string all_servers_dirname =
"/REPLACE_WITH_ABSOLUTE_PATH/";) if you choose extern; also replace the current
relative default ("../all_servers/") with a clearly invalid sentinel or empty
string (e.g., "" or "/REPLACE_WITH_ABSOLUTE_PATH/") so admins must set an
absolute path before building.
In `@lib/autolab/multi_server.cpp`:
- Line 9: Copy lib/all_servers_dirname.h.template to lib/all_servers_dirname.h
before building and ensure the project’s build/install instructions mention this
step; then edit the copied header so the value used by the MultiServer
constructor (MultiServer::MultiServer or the variable/constant defined in
all_servers_dirname.h) is an absolute path (starts with '/') instead of the
current "../all_servers/" relative path to satisfy the constructor’s requirement
and prevent runtime exceptions; update the template file or repo README to
document that the header must be created from the template and that the path
must be absolute.
- Around line 66-69: The check `if (num_read <= 0)` is incorrect because
`num_read` is a size_t (unsigned) and `<= 0` only matches zero; update the check
around the `read_file` result handling in multi_server.cpp (the `num_read`
variable used where you log "Not able to read file in directory") to explicitly
test for `num_read == 0` if you intend to detect zero bytes read, or change
`num_read` to a signed type (or have `read_file` return a sentinel/error code)
if you expect negative error codes — pick one approach and update the
conditional and error handling accordingly.
- Around line 137-143: In get_server_from_course (a const AllServers method)
avoid calling the global g_all_servers; instead use this->get_all_courses() or
iterate the member m_server_list directly to gather course names and log them
(e.g., call this->get_all_courses() or loop over m_server_list), then log via
Logger and throw Autolab::InvalidInputException as before; replace the
g_all_servers reference with this to remove the global-instance dependency.
- Around line 56-106: The opendir() handle stored in dir is never closed (DIR
*dir = opendir(...)), causing a file descriptor leak; ensure closedir(dir) is
called before every exit point from the loop and after the loop completes —
i.e., call closedir(dir) just before throwing InvalidInputException in all error
branches (num_read<=0, duplicate server/course, malformed JSON) and once after
the while loop finishes processing entries so the directory is always closed
whether you push ServerInfo into m_server_list or throw from functions like
read_file/get_string_force/require_is_array.
In `@lib/autolab/raw_client.cpp`:
- Around line 493-496: The docstring above RawClient::get_courses contains an
incomplete "@pre The user has" line; either finish it with the correct
precondition (for example "@pre The user has a valid authenticated session and
appropriate permissions" or "@pre server_info is initialized") using proper
Doxygen style, or remove the incomplete "@pre" line entirely so the comment is
not left dangling; update only the comment block immediately preceding the
RawClient::get_courses definition to keep the documentation consistent.
In `@src/app_credentials_andrew.h`:
- Around line 10-16: The header declares and initializes a const std::string
(server_domain) but does not include the <string> header, causing compilation
errors; add the missing `#include` <string> immediately after the header guard
(after `#ifndef/`#define) so std::string is defined before server_domain is used
and keep the existing header guard (AUTOLAB_APP_CREDENTIALS_H_) and const
std::string server_domain unchanged.
In `@src/cmd/cmdimp.cpp`:
- Around line 89-98: check_for_setup currently calls
g_all_servers.get_server_from_course(course_name) which can throw
Autolab::InvalidInputException; catch that exception around the call (or the
whole body), log a user-friendly error via Logger::fatal (e.g., "Course not
found: <course_name>" plus the exception message) and return false instead of
letting the exception propagate. Ensure you still return the existing "No user
set up..." message when client.has_auth_for_server(server_name) is false, and
reference the function check_for_setup and the method get_server_from_course to
locate where to add the try/catch.
In `@src/context_manager/context_manager.cpp`:
- Around line 46-54: get_token_cache_file_full_path currently appends raw
server_name into the filename which can contain filesystem-unsafe characters;
replace that by sanitizing or hashing server_name before appending. Add a helper
(e.g., sanitize_server_name or hash_server_name) and call it from
get_token_cache_file_full_path so that only safe characters (or a hex hash) are
used; keep token_cache_filename and get_cred_dir_full_path unchanged and use the
sanitized/hashed value in place of server_name to avoid invalid filenames for
write_file/read_file.
In `@src/main.cpp`:
- Around line 81-82: user_setup currently calls
g_all_servers.get_server_from_course(course_name) which can throw
Autolab::InvalidInputException; wrap that call in a try/catch that catches
Autolab::InvalidInputException (and optionally const std::exception& for
safety), log a clear error via existing logging mechanism, and perform a clean
exit/return instead of letting the exception propagate. Specifically, surround
the call that assigns Autolab::ServerInfo target_server_info and the subsequent
std::string target_server = target_server_info.server_name with a try block,
catch Autolab::InvalidInputException& e (use e.what() in the log), and handle by
logging the error and exiting/returning from user_setup gracefully.
---
Nitpick comments:
In `@include/autolab/client.h`:
- Around line 35-36: The parameter name server_info__ in the declaration of
get_user_info(User &user, const ServerInfo& server_info__) is inconsistent;
rename the parameter to server_info for consistency with other methods (e.g.,
use const ServerInfo& server_info) and update the corresponding
definition/implementation and any call sites that use server_info__ to use
server_info instead so all references to ServerInfo use the same identifier.
In `@include/autolab/multi_server.h`:
- Around line 1-2: The header guard _MULTI_SERVER_H_ uses a reserved identifier
(leading underscore + uppercase); replace it with a non-reserved macro
throughout the file (update both the `#ifndef` and `#define` that reference
_MULTI_SERVER_H_ and the corresponding `#endif` comment if present) — for example
use MULTI_SERVER_H or a project-scoped name like
AUTOLAB_MULTI_SERVER_H_INCLUDED, ensuring all references in
include/autolab/multi_server.h match the new symbol.
- Around line 18-20: AllServers currently exposes sensitive credentials via the
public member m_server_list (vector<ServerInfo>), so make m_server_list private
and provide controlled accessors: add a const getter like get_servers() that
returns either a const reference or a sanitized copy (omitting client_secret)
and a separate method get_server_secret(id) or fetch_secret(id) to retrieve
secrets under controlled checks; update any code referencing
AllServers::m_server_list to use these methods and keep mutation confined to
methods like add_server(...) / update_server(...) on the AllServers class to
validate inputs and avoid direct external modification.
In `@include/autolab/raw_client.h`:
- Around line 73-74: The parameter name for ServerInfo is inconsistent (some
declarations use server_info__ while others use server_info); standardize to a
single name (recommend server_info) across the header and corresponding
implementation files by renaming the parameter in all function declarations and
matching definitions (e.g., get_user_info(rapidjson::Document &result, const
ServerInfo& server_info), get_courses(...), and the functions referenced around
the other declarations) so signatures and parameter names are consistent
throughout the codebase.
In `@lib/autolab/multi_server.cpp`:
- Line 96: The for-loop uses a signed int `course_idx` compared against
`courses_value.Size()` (an unsigned rapidjson::SizeType), causing a
signed/unsigned warning; change the loop index to an unsigned type such as
`rapidjson::SizeType` (or `decltype(courses_value.Size())` / `std::size_t`) in
the loop initializing `course_idx` to eliminate the signed/unsigned mismatch
when iterating over `courses_value`.
- Line 60: The current check using entry->d_type != DT_REG is not portable;
update the directory iteration (the readdir loop that accesses entry->d_type and
entry->d_name) to handle DT_UNKNOWN by falling back to stat/lstat: if
entry->d_type == DT_REG accept as regular file, if entry->d_type == DT_UNKNOWN
construct the full pathname (combine the directory path and entry->d_name) and
call stat() (or lstat() if symlink behavior is desired) and use
S_ISREG(st.st_mode) to decide whether to continue; only skip non-regular files
when both the d_type check and the stat fallback indicate the entry is not a
regular file.
- Around line 183-191: Replace the inconsistent std::runtime_error thrown in
AllAuthInfo::get_access_token_from_server with the same InvalidInputException
used by get_server_from_course and get_server_from_name: after failing to find a
matching AuthInfo, construct and throw an InvalidInputException with a clear
message (e.g., "Could not find access token for server '<server_name>'; please
rerun autolab setup for this course") and ensure any logging remains consistent
with other lookup failures (remove or align Logger::fatal if those functions
don't use it).
In `@lib/autolab/raw_client.cpp`:
- Around line 12-13: The relative include "#include
\"../../src/context_manager/context_manager.h\"" is fragile; replace it with a
stable include and/or add the header's parent directory to the build include
paths: update the CMake target that builds lib/autolab/raw_client.cpp (or the
library target that owns it) to add target_include_directories(...) pointing to
the directory that contains context_manager/context_manager.h, then change the
source to include it as "#include <context_manager/context_manager.h>" (or move
the header to a common include dir and adjust includes accordingly) so the
compile no longer depends on "../../" relative paths.
- Around line 154-160: The code uses deprecated form APIs (curl_formadd,
CURLFORM_COPYNAME, CURLFORM_FILE, CURLOPT_HTTPPOST and curl_formfree); replace
this with the curl_mime API by creating a mime handle via curl_mime_init(curl),
add a part with curl_mime_name(mimepart, "submission[file]") and attach the file
using curl_mime_filedata(mimepart, rstate->upload_filename.c_str()), then set
the mime on the easy handle with curl_easy_setopt(curl, CURLOPT_MIMEPOST, mime)
and free the mime with curl_mime_free(mime) when done; update references in this
file from the old symbols
(curl_formadd/lastptr/CURLFORM_*/CURLOPT_HTTPPOST/curl_formfree) to the
corresponding curl_mime_* calls to silence deprecation warnings and ensure
compatibility.
In `@README.md`:
- Around line 39-44: Update the phrasing in README.md where the sentence begins
"Inside of this directory..." to remove the redundant "of" so it reads "Inside
this directory, insert files that follow the format as can be seen in
`lib/server_info_example.txt`"—locate the sentence in the README and replace
"Inside of" with "Inside" to satisfy the static analysis style hint.
In `@src/cmd/cmdimp.h`:
- Around line 4-9: The header declares Autolab::ServerInfo in the
perform_device_flow signature but doesn't include its defining header; add an
explicit include for the header that defines Autolab::ServerInfo (e.g. include
"autolab/multi_server.h") at the top of the file so the symbol is resolved
directly rather than relying on transitive includes; update the includes near
init_autolab_client and perform_device_flow declarations accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c36bf06-4bde-4716-a070-8c80e2fa168d
⛔ Files ignored due to path filters (1)
.vscode/settings.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
.gitignoreCMakeLists.txtREADME.mdbinaries/autolab_devinclude/autolab/autolab.hinclude/autolab/client.hinclude/autolab/multi_server.hinclude/autolab/raw_client.hlib/all_servers_dirname.h.templatelib/autolab/CMakeLists.txtlib/autolab/client.cpplib/autolab/multi_server.cpplib/autolab/raw_client.cpplib/server_info_example.txtsrc/app_credentials_andrew.hsrc/cmd/cmdimp.cppsrc/cmd/cmdimp.hsrc/context_manager/context_manager.cppsrc/context_manager/context_manager.hsrc/main.cpp
Comment on lines
+27
to
+29
| // Returns "" if there is no such course | ||
| ServerInfo get_server_from_course(const std::string& course_name) const; | ||
| ServerInfo get_server_from_name(const std::string& server_name) const; |
Contributor
There was a problem hiding this comment.
Misleading comment: return type is ServerInfo, not "".
The comment says "Returns "" if there is no such course" but the function returns ServerInfo. Looking at the implementation in multi_server.cpp, the function actually throws an exception when the course is not found. Update the comment to reflect the actual behavior.
Suggested fix
- // Returns "" if there is no such course
- ServerInfo get_server_from_course(const std::string& course_name) const;
- ServerInfo get_server_from_name(const std::string& server_name) const;
+ // Throws InvalidInputException if the course is not found
+ ServerInfo get_server_from_course(const std::string& course_name) const;
+ // Throws InvalidInputException if the server name is not found
+ ServerInfo get_server_from_name(const std::string& server_name) const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/autolab/multi_server.h` around lines 27 - 29, The comment for
get_server_from_course (and similarly get_server_from_name) is incorrect: these
functions return ServerInfo and, per the implementation in multi_server.cpp,
throw an exception when the requested server/course is not found; update the
comment to state that they return a ServerInfo on success and throw an exception
(specify exception type if known) when no matching course/server exists so the
declaration matches the actual behavior.
Comment on lines
+46
to
+48
| // Returns "" if it does not exist | ||
| std::string get_access_token_from_server(const std::string& server_name); | ||
| std::string get_refresh_token_from_server(const std::string& server_name); |
Contributor
There was a problem hiding this comment.
Misleading comment: these functions throw or return empty string.
Similar to get_server_from_course, update comments to reflect that get_access_token_from_server throws on failure (per implementation), while get_refresh_token_from_server returns empty string.
Suggested fix
- // Returns "" if it does not exist
- std::string get_access_token_from_server(const std::string& server_name);
+ // Throws std::runtime_error if it does not exist
+ std::string get_access_token_from_server(const std::string& server_name);
+ // Returns "" if it does not exist
std::string get_refresh_token_from_server(const std::string& server_name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Returns "" if it does not exist | |
| std::string get_access_token_from_server(const std::string& server_name); | |
| std::string get_refresh_token_from_server(const std::string& server_name); | |
| // Throws std::runtime_error if it does not exist | |
| std::string get_access_token_from_server(const std::string& server_name); | |
| // Returns "" if it does not exist | |
| std::string get_refresh_token_from_server(const std::string& server_name); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/autolab/multi_server.h` around lines 46 - 48, Update the misleading
comment above the token accessors: change the comment for
get_access_token_from_server to state it throws on failure (instead of returning
""), and update the comment for get_refresh_token_from_server to explicitly
state it returns an empty string when not found; reference the functions
get_access_token_from_server(const std::string& server_name) and
get_refresh_token_from_server(const std::string& server_name) so callers know
the different error behaviors.
| bool get_token_from_authorization_code(std::string authorization_code); | ||
| bool perform_token_refresh(); | ||
| bool save_tokens_from_response(rapidjson::Document &response, const std::string& server_name); | ||
| bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name); |
Contributor
There was a problem hiding this comment.
Misleading parameter name: server_name has type ServerInfo.
The parameter is named server_name but its type is ServerInfo, which is confusing. Rename to server_info for clarity.
Suggested fix
- bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name);
+ bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_info);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_name); | |
| bool get_token_from_authorization_code(std::string authorization_code, const ServerInfo& server_info); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/autolab/raw_client.h` at line 135, Rename the misleading parameter
name server_name to server_info in the declaration of
get_token_from_authorization_code(std::string authorization_code, const
ServerInfo& server_info) and update the corresponding definition/implementation
and all call sites to use server_info; ensure any documentation/comments and
variable usages inside the get_token_from_authorization_code function that
referenced server_name are updated to the new identifier to match the ServerInfo
type.
Comment on lines
+1
to
+10
| #ifndef _ALL_SERVERS_DIRNAME_H_ | ||
| #define _ALL_SERVERS_DIRNAME_H_ | ||
|
|
||
| #include <string> | ||
|
|
||
| /* Fill this in with where all the server text files are stored. Make sure it | ||
| is an absolute path. */ | ||
| std::string all_servers_dirname = "../all_servers/"; | ||
|
|
||
| #endif /* _ALL_SERVERS_DIRNAME_H_ */ No newline at end of file |
Contributor
There was a problem hiding this comment.
Static initialization order fiasco risk.
Per context snippet at lib/autolab/multi_server.cpp:205, g_all_servers is constructed at global scope using all_servers_dirname. Since both are globals in different translation units, there's no guaranteed initialization order—all_servers_dirname may not be initialized when g_all_servers's constructor runs.
Consider using a function that returns a reference to a local static (lazy initialization) to guarantee order:
inline const std::string& get_all_servers_dirname() {
static std::string dirname = "/path/to/servers/";
return dirname;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/all_servers_dirname.h.template` around lines 1 - 10, all_servers_dirname
is a global std::string that can be used before it's initialized by
g_all_servers in another TU, causing static initialization order problems;
replace the global variable with an accessor function (e.g.,
get_all_servers_dirname()) that returns a const std::string& initialized as a
local static (lazy init) and update all call sites (including the g_all_servers
construction in multi_server.cpp) to use get_all_servers_dirname() instead of
the global variable to ensure safe initialization order.
Comment on lines
+6
to
+8
| /* Fill this in with where all the server text files are stored. Make sure it | ||
| is an absolute path. */ | ||
| std::string all_servers_dirname = "../all_servers/"; |
Contributor
There was a problem hiding this comment.
Multiple definition (ODR) violation: Variable defined in header.
Defining all_servers_dirname in a header will cause linker errors if this header is included in multiple translation units. Either:
- Declare with
externand define in a.cppfile, or - Use
inline(C++17) to allow multiple definitions.
Additionally, consider making the default value clearly invalid (e.g., empty string or a sentinel) to force administrators to set it, rather than a relative path that will fail at runtime with "path should be an absolute path" error.
🐛 Proposed fix using extern declaration
In the header template:
-std::string all_servers_dirname = "../all_servers/";
+extern std::string all_servers_dirname;Then define in a corresponding .cpp file (e.g., all_servers_dirname.cpp):
`#include` "all_servers_dirname.h"
// Administrator: set this to an absolute path before building
std::string all_servers_dirname = "/REPLACE_WITH_ABSOLUTE_PATH/";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/all_servers_dirname.h.template` around lines 6 - 8, The header currently
defines the variable all_servers_dirname which causes an ODR violation; change
the header to only declare the symbol (e.g., extern std::string
all_servers_dirname;) or mark it inline (C++17) to allow header definitions, and
move the single definitive definition into a .cpp translation unit (e.g., define
std::string all_servers_dirname = "/REPLACE_WITH_ABSOLUTE_PATH/";) if you choose
extern; also replace the current relative default ("../all_servers/") with a
clearly invalid sentinel or empty string (e.g., "" or
"/REPLACE_WITH_ABSOLUTE_PATH/") so admins must set an absolute path before
building.
Comment on lines
+493
to
+496
| /** | ||
| * @pre The user has | ||
| */ | ||
| void RawClient::get_courses(rapidjson::Document &result, const ServerInfo& server_info) { |
Contributor
There was a problem hiding this comment.
Incomplete docstring.
The docstring @pre The user has on line 494 is incomplete and should either be finished or removed.
📝 Proposed fix
-/**
- * `@pre` The user has
- */
void RawClient::get_courses(rapidjson::Document &result, const ServerInfo& server_info) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @pre The user has | |
| */ | |
| void RawClient::get_courses(rapidjson::Document &result, const ServerInfo& server_info) { | |
| void RawClient::get_courses(rapidjson::Document &result, const ServerInfo& server_info) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/autolab/raw_client.cpp` around lines 493 - 496, The docstring above
RawClient::get_courses contains an incomplete "@pre The user has" line; either
finish it with the correct precondition (for example "@pre The user has a valid
authenticated session and appropriate permissions" or "@pre server_info is
initialized") using proper Doxygen style, or remove the incomplete "@pre" line
entirely so the comment is not left dangling; update only the comment block
immediately preceding the RawClient::get_courses definition to keep the
documentation consistent.
src/app_credentials_andrew.h
Outdated
Comment on lines
+10
to
+16
| #ifndef AUTOLAB_APP_CREDENTIALS_H_ | ||
| #define AUTOLAB_APP_CREDENTIALS_H_ | ||
|
|
||
| /* The domain of the autolab service. Do NOT include the trailing slash. | ||
| * Must use 'https' instead of 'http' in production. | ||
| */ | ||
| const std::string server_domain = "https://autolab.andrew.cmu.edu"; |
Contributor
There was a problem hiding this comment.
Missing #include <string> causes compilation failure.
The file uses std::string but doesn't include <string>. This is confirmed by the static analysis errors. Add the missing include after the header guard.
🐛 Proposed fix
`#ifndef` AUTOLAB_APP_CREDENTIALS_H_
`#define` AUTOLAB_APP_CREDENTIALS_H_
+#include <string>
+
/* The domain of the autolab service. Do NOT include the trailing slash.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #ifndef AUTOLAB_APP_CREDENTIALS_H_ | |
| #define AUTOLAB_APP_CREDENTIALS_H_ | |
| /* The domain of the autolab service. Do NOT include the trailing slash. | |
| * Must use 'https' instead of 'http' in production. | |
| */ | |
| const std::string server_domain = "https://autolab.andrew.cmu.edu"; | |
| `#ifndef` AUTOLAB_APP_CREDENTIALS_H_ | |
| `#define` AUTOLAB_APP_CREDENTIALS_H_ | |
| `#include` <string> | |
| /* The domain of the autolab service. Do NOT include the trailing slash. | |
| * Must use 'https' instead of 'http' in production. | |
| */ | |
| const std::string server_domain = "https://autolab.andrew.cmu.edu"; |
🧰 Tools
🪛 Clang (14.0.6)
[error] 16-16: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 16-16: expected identifier or '('
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app_credentials_andrew.h` around lines 10 - 16, The header declares and
initializes a const std::string (server_domain) but does not include the
<string> header, causing compilation errors; add the missing `#include` <string>
immediately after the header guard (after `#ifndef/`#define) so std::string is
defined before server_domain is used and keep the existing header guard
(AUTOLAB_APP_CREDENTIALS_H_) and const std::string server_domain unchanged.
Comment on lines
+89
to
+98
| bool check_for_setup(const std::string& course_name) { | ||
| const std::string& server_name = g_all_servers.get_server_from_course(course_name).server_name; | ||
| bool result = client.has_auth_for_server(server_name); | ||
| if (!result) { | ||
| Logger::fatal << "No user set up on this client yet." << Logger::endl | ||
| << Logger::endl | ||
| << "Please run 'autolab setup' to setup your Autolab account." << Logger::endl; | ||
| } | ||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
Unhandled InvalidInputException in check_for_setup.
g_all_servers.get_server_from_course(course_name) on line 90 throws Autolab::InvalidInputException if the course is not found. This exception propagates up to the caller commands, which don't catch it, leading to unhandled exception termination.
Consider catching the exception here and returning false with an appropriate error message.
🐛 Proposed fix
bool check_for_setup(const std::string& course_name) {
- const std::string& server_name = g_all_servers.get_server_from_course(course_name).server_name;
+ std::string server_name;
+ try {
+ server_name = g_all_servers.get_server_from_course(course_name).server_name;
+ } catch (Autolab::InvalidInputException &e) {
+ // Error message already printed by get_server_from_course
+ return false;
+ }
bool result = client.has_auth_for_server(server_name);
if (!result) {
Logger::fatal << "No user set up on this client yet." << Logger::endl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cmdimp.cpp` around lines 89 - 98, check_for_setup currently calls
g_all_servers.get_server_from_course(course_name) which can throw
Autolab::InvalidInputException; catch that exception around the call (or the
whole body), log a user-friendly error via Logger::fatal (e.g., "Course not
found: <course_name>" plus the exception message) and return false instead of
letting the exception propagate. Ensure you still return the existing "No user
set up..." message when client.has_auth_for_server(server_name) is false, and
reference the function check_for_setup and the method get_server_from_course to
locate where to add the try/catch.
Comment on lines
+46
to
54
| std::string get_token_cache_file_full_path(const std::string& server_name) { | ||
| std::string token_cache_file_full_path; | ||
|
|
||
| token_cache_file_full_path.append(get_cred_dir_full_path()); | ||
| token_cache_file_full_path.append("/"); | ||
| token_cache_file_full_path.append(token_cache_filename); | ||
| token_cache_file_full_path.append("_"); | ||
| token_cache_file_full_path.append(server_name); | ||
| return token_cache_file_full_path; |
Contributor
There was a problem hiding this comment.
Potential issue: server_name may contain filesystem-unsafe characters.
When server_name is derived from URLs (e.g., "https://example.com"), it may contain characters like /, :, or ? that are invalid or problematic in filenames on various platforms. This could cause write_file or read_file to fail.
Consider sanitizing server_name before using it in the file path, or use a hash/encoding of the server name.
🛡️ Proposed fix to sanitize server_name
std::string get_token_cache_file_full_path(const std::string& server_name) {
std::string token_cache_file_full_path;
+
+ // Sanitize server_name for use in filename
+ std::string safe_name = server_name;
+ for (char& c : safe_name) {
+ if (c == '/' || c == ':' || c == '?' || c == '&' || c == '=' || c == '\\') {
+ c = '_';
+ }
+ }
token_cache_file_full_path.append(get_cred_dir_full_path());
token_cache_file_full_path.append("/");
token_cache_file_full_path.append(token_cache_filename);
token_cache_file_full_path.append("_");
- token_cache_file_full_path.append(server_name);
+ token_cache_file_full_path.append(safe_name);
return token_cache_file_full_path;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| std::string get_token_cache_file_full_path(const std::string& server_name) { | |
| std::string token_cache_file_full_path; | |
| token_cache_file_full_path.append(get_cred_dir_full_path()); | |
| token_cache_file_full_path.append("/"); | |
| token_cache_file_full_path.append(token_cache_filename); | |
| token_cache_file_full_path.append("_"); | |
| token_cache_file_full_path.append(server_name); | |
| return token_cache_file_full_path; | |
| std::string get_token_cache_file_full_path(const std::string& server_name) { | |
| std::string token_cache_file_full_path; | |
| // Sanitize server_name for use in filename | |
| std::string safe_name = server_name; | |
| for (char& c : safe_name) { | |
| if (c == '/' || c == ':' || c == '?' || c == '&' || c == '=' || c == '\\') { | |
| c = '_'; | |
| } | |
| } | |
| token_cache_file_full_path.append(get_cred_dir_full_path()); | |
| token_cache_file_full_path.append("/"); | |
| token_cache_file_full_path.append(token_cache_filename); | |
| token_cache_file_full_path.append("_"); | |
| token_cache_file_full_path.append(safe_name); | |
| return token_cache_file_full_path; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context_manager/context_manager.cpp` around lines 46 - 54,
get_token_cache_file_full_path currently appends raw server_name into the
filename which can contain filesystem-unsafe characters; replace that by
sanitizing or hashing server_name before appending. Add a helper (e.g.,
sanitize_server_name or hash_server_name) and call it from
get_token_cache_file_full_path so that only safe characters (or a hex hash) are
used; keep token_cache_filename and get_cred_dir_full_path unchanged and use the
sanitized/hashed value in place of server_name to avoid invalid filenames for
write_file/read_file.
Comment on lines
+81
to
+82
| Autolab::ServerInfo target_server_info = g_all_servers.get_server_from_course(course_name); | ||
| std::string target_server = target_server_info.server_name; |
Contributor
There was a problem hiding this comment.
Unhandled InvalidInputException when course is not found.
g_all_servers.get_server_from_course(course_name) throws Autolab::InvalidInputException when the course is not in any server's configuration (see lib/autolab/multi_server.cpp:62-77). This exception is not caught in user_setup(), causing an unhandled exception termination.
While the function does log an error message before throwing, the unhandled exception will produce an ugly crash rather than a clean exit.
🐛 Proposed fix to catch the exception
+ Autolab::ServerInfo target_server_info;
+ try {
+ target_server_info = g_all_servers.get_server_from_course(course_name);
+ } catch (Autolab::InvalidInputException &e) {
+ return 1;
+ }
- Autolab::ServerInfo target_server_info = g_all_servers.get_server_from_course(course_name);
std::string target_server = target_server_info.server_name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.cpp` around lines 81 - 82, user_setup currently calls
g_all_servers.get_server_from_course(course_name) which can throw
Autolab::InvalidInputException; wrap that call in a try/catch that catches
Autolab::InvalidInputException (and optionally const std::exception& for
safety), log a clear error via existing logging mechanism, and perform a clean
exit/return instead of letting the exception propagate. Specifically, surround
the call that assigns Autolab::ServerInfo target_server_info and the subsequent
std::string target_server = target_server_info.server_name with a try block,
catch Autolab::InvalidInputException& e (use e.what() in the log), and handle by
logging the error and exiting/returning from user_setup gracefully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As part of our migration to AWS, Autolab will be partitioned into different servers managed by different departments. This requires changing the Autolab CLI to support this. To also add additional flexibility and prevent administrators from having to recompile the binary excessively, this is done by changing the client IDs and secret to be runtime queries in a directory that can be added to freely.
New instructions for administrators are in the README. In theory, one only needs to compile the binary once for all users.
Companion PR: autolab/Autolab#2315